-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
dev/core#1913 Allow for schemas to be added by extensions if they are… #17986
dev/core#1913 Allow for schemas to be added by extensions if they are… #17986
Conversation
(Standard links)
|
@seamuslee001 are those tests still valid? |
@eileenmcnaughton Yes one of the test is showing that with Grant Fields added via the UF Fields hook then sepecifiying GrantModel works without any extra code which it doesn't do right now, the 2nd test is showing that if you pass in a model entity that has no fields e.g. in that case PledgeModel then it should still correctly error (i.e. the expectedException is thrown) |
OK - I'm happy with that then - it is using an existing hook, code changes seem sensible & based on reviewer input and it's tested |
Actually @seamuslee001 one more thing - can you add some explanatory doc blocks to the test functions? |
… in the format of <entityname>Model Update test doc blocks
3790677
to
22877e0
Compare
Hopefully should be fine now @eileenmcnaughton |
unrelated fail |
@seamuslee001 This needs some kind of documentation - if I stumbled upon that bit of code that's added here I would have no idea what is a "Model" or what it is doing? |
Hmm - I'm happy for it to work - but in no hurry to encourage anyone else to use it by documenting it |
@eileenmcnaughton At the very least we should have a code comment otherwise it just makes it harder for any poor dev refactoring this code in the future. |
… in the format of Model
Overview
This is an alternate PR to #17971 which aims to achieve the same effect without the new hook
Before
Adding in an entity GrandModel for UF ProfileSchema doesn't work without a core override
After
Does work without a core override as long as there are availabe fields added
ping @colemanw @eileenmcnaughton @monishdeb