Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Something went wrong I think.
We ended up with a (mutable) static and I think we wanted an (immutable) constant.
Also because it's protected, can't use it elsewhere - whilst a constant is public by definition.
Proposed resolution
Fix
Remaining tasks
Review
User interface changes
None
API changes
I don't think so
Beta phase evaluation
| Issue category | Bug because API is wrong without it in my opinion (should not be mutable, should be public) | Issue priority | Major because blocks #2403873: FileFormatterBase does not retain unsaved entities (files) which is major because it means data-loss |
|---|---|
| Disruption | Could only possibly impact on dynamic_entity_reference (which I'm a maintainer of) - so none. |
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2403793-14.patch | 907 bytes | jibran |
Comments
Comment #1
larowlanComment #2
dawehnerLooks totally reasonable!
Comment #4
larowlanThis blocks #2403873: FileFormatterBase does not retain unsaved entities (files) which is major (data-loss) so bumping
Comment #5
alexpottCommitted 4243b60 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation.
Comment #7
plachPlease revert this commit: the previous solution was meant exactly to avoid exposing the value outside the class, as it's an implementation detail. We have a proper API to check the value:
EntityReferenceItem::hasNewEntity().I'd use a protected constant if they were available...
Comment #8
larowlanShould we set it to RTBC to get committer eyes?
thanks for explaining - should we add a comment documenting as such to prevent this happening again?
Comment #9
plachGood idea :)
Comment #10
alexpottreverted
Comment #12
plachComment #13
dawehnerSorry, I could have maybe known that from passive listening in ghent.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedAdded docs in #2404221: Document that EntityReferenceItem uses a static to protect access to a constant
Comment #15
jibranHere is the doc updated patch. Just for the record I knew this @fago told @plach in IRC why we should make this a static. I thought I could find something in #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities but there is nothing this is the patch where @plach updated it from constant to static #2232477-104: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities.
Comment #16
jibranSorry for the cross post I'll post the patch in other issue.