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

dev/core#1913 Allow for schemas to be added by extensions if they are… #17986

Merged

Conversation

seamuslee001
Copy link
Contributor

… 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

@civibot
Copy link

civibot bot commented Jul 28, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 are those tests still valid?

@seamuslee001
Copy link
Contributor Author

@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)

@eileenmcnaughton
Copy link
Contributor

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

@eileenmcnaughton
Copy link
Contributor

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
@seamuslee001
Copy link
Contributor Author

Hopefully should be fine now @eileenmcnaughton

@eileenmcnaughton
Copy link
Contributor

unrelated fail

@eileenmcnaughton eileenmcnaughton merged commit 2825155 into civicrm:master Jul 29, 2020
@eileenmcnaughton eileenmcnaughton deleted the dev_core_1913_alternate branch July 29, 2020 12:50
@mattwire
Copy link
Contributor

@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?

@eileenmcnaughton
Copy link
Contributor

Hmm - I'm happy for it to work - but in no hurry to encourage anyone else to use it by documenting it

@mattwire
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants