Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include silverstripe core files into roots #10893

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Jul 29, 2023

Fixes #10892

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a unit test to cover the scenario here. Could be as simple as testing that Email sub classes are being returned now.

@lekoala
Copy link
Contributor Author

lekoala commented Jul 31, 2023

@maxime-rainville hi maxime, so i did try to add a quick test but it's a bit more complicated than expected and i'd like your feedback and how you would like things done

  • i've tried to add the assertion to ClassInfoTest::testSubclassesFor but that doesn't work since there is no proper manifest being built here (so it's empty). The current test only works because it's working with manually registered dataobjects. And its seems adding a whole manifest in the test just for that assertion would be a lot of code.

an alternative would be to:

  • add to the emailtest, but again there are no manifest and that would means setting up a whole manifest in the test just for that
  • or add the test to the ClassManifestTest and add new fixtures, but again, that seems a bit sad since we already have a email subclass in the tests

tests/php/Core/ClassInfoTest.php Outdated Show resolved Hide resolved
@maxime-rainville
Copy link
Contributor

I just rebased the PR to squsah all the commits into one. Also the build hadn't run in that last commit for some reason. So that should re-trigger the workflow.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged on green.

@maxime-rainville maxime-rainville merged commit 6ed50b5 into silverstripe:5.0 Aug 1, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants