-
Notifications
You must be signed in to change notification settings - Fork 70
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
VACMS-16457: Update/patch entity_browser module #16542
Conversation
@edmund-dunn changes look good but there appears to be a cypress test failure. |
@dsasser that test is no longer needed. Per https://www.drupal.org/project/entity_browser/issues/3074457 that is now part of entity_browser. |
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.
This is fine to merge from my perspective once merge conflicts are resolved 🙂
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 patch for entity_browser is a bit weird in that it is loading entities that ostensibly should already be there. Swirt had a similar sentiment on the d.o issue.
@dsasser that was a good call. I was able to remove that patch. The patch on entity_borwser_tabloe to bring it in line with the changes to entity_browser seemed to have fixed our issues. |
patches/3408216-error-call-to-member-function-getstorage-on-null.patch
Outdated
Show resolved
Hide resolved
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 am happy that original patch has been removed.
QA Steps
Review instances of entity browser:
There are only 2 instances of entity_browser_table being used on Facilities products do my QA is limited to those. In the absence of actual QA steps on the PR, I am assuming that CMS team and PW team have reviewed the other instances.
Added and removed phone numbers fine. Allowed number of entries still work
Description
Closes #16457.
This updates the
entity_browser
module and patches it and theentity_browser_tables
module. There were breaking changes inentity_browser
see https://www.drupal.org/project/entity_browser/issues/3407702 for reference.Testing done
Tested locally. Cypress tests that failed in the dependabot PR passed locally.
Screenshots
QA steps
What needs to be checked to prove this works?
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?
Mostly this would be considered good to go if autoated tests pass. However in light of the information on breaking changes we need each product team to review to make sure that none of their custom code is effected by those breaking changes.
Definition of Done
Select Team for PR review
CMS Team
Public websites
Facilities
User support
Accelerated Publishing
Is this PR blocked by another PR?
DO NOT MERGE
Does this PR need review from a Product Owner
Needs PO review
CMS user-facing announcement
Is an announcement needed to let editors know of this change?