-
-
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
Add EntityPageTrait #14399
Add EntityPageTrait #14399
Conversation
(Standard links)
|
"reset=1&id={$cid}" | ||
); | ||
if ($this->getContext() == 'dashboard') { | ||
$url = CRM_Utils_System::url('civicrm/user', "reset=1&id={$this->getContactId()}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if a function call inside curly places in a quote is valid php - I'm guessing maybe it is - but stylistically we have no precedent for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's new to php5, but it works fine; I've done it myself in some spots.
https://www.php.net/manual/en/language.types.string.php#language.types.string.parsing.complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - I guess it's ok then - will merge
@mattwire I worked through this & it all looks good. I loaded the relationship view form & it seemed unchanged. I just made one minor comment & with that solved this is mergeable IMHO |
*/ | ||
protected function isEditContext() { | ||
return ($this->getAction() & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isDeleteContext
et al. are confusing to me because the comments mention a form, but isn't this a page class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh - our separation of concerns huh
@mattwire I'm curious to know how many page classes you've tried this with to see how broadly applicable it is and how many quirky differences would need resolved. |
@colemanw I suspect quite a few - as we've seen with work on the entity form trait & setting form trait - but a lot of the resolutions have been stdisation & metadata cleanup & moving stuff out of the BAO - all the same things we need to do to prepare for a new form layer |
Overview
Related to #14184
There is lot's of inconsistent but shared code on pages. This does what we did for forms (with EntityFormTrait) but for pages. This PR converts
CRM_Contact_Page_View_Relationship
and adds the new trait.Before
No easy way to share common "page" functionality without adding to
CRM_Core_Page
After
Every page that is working with an entity could be converted to use
EntityPageTrait
bringing consistency to parameters / assignments.Technical Details
Basically per EntityFormTrait where appropriate. Note this allows us to deprecate the dual
$this->_contactId
and$this->_contactID
that we have on some pages.Comments
@eileenmcnaughton I imagine this is one you will like in some way.