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
Quoting #2016679: Expand Entity Type interfaces to provide methods, protect the properties:
Public properties on our entities allow APIs to be circumvented, and increases the likelihood of introducing bugs because an entity object may not be in a reliable or accurate state. (Additionally, ->something->value is cumbersome DX.)
The result of that issue was:
1) Content entities now have getters and setters which act as wrappers around the get() and set() methods.
2) Config entities now have getters and setters, and don't care about get() and set() at all.
This means that unlike content entities, config entities now have two ways (and code paths) to get or modify a property.
This makes all benefits of the getters and setters moot, since they can be bypassed (and actually are, by CMI, forms, entity_create(), etc).
EDIT: There's also a third way, the constructor, being fixed in #2399999: ConfigEntityBase::__construct() should use set() to populate the values.
For getters, the main benefit is ensuring BC:
- A config entity has first_name and last_name properties, which get deprecated (removed from the forms, kept in schema) in favor of a more generic name property.
The getName() accessor can see if first_name and last_name are available, and use them to construct and return the name. But ->get('name') will bypass that.
For setters, the benefit is data consistency:
Imagine a workflow entity type that has an array of states:
protected $states = array();
public function setStates(array $states) {
$this->states = $states;
return $this;
}
This ensures getStates() always returns an array, and the data is always valid. But set('states', 'banana') still works. Sure, it will fail on save when the schema is consulted, but other code has plenty of time to crash until then.
Solution
Change ConfigEntityBase's get() and set() to invoke the getters and setters when found.
The property name is camelized, the method's existence is checked (for first_name we search for getFirstName() and setFirstName()), if found it is used.
Otherwise the old behavior is kept (the property is modified directly).
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | 2399965-nr-bot.txt | 144 bytes | needs-review-queue-bot |
| #42 | reroll_diff_40-42.txt | 5.66 KB | Medha Kumari |
| #42 | 2399965-42.patch | 9.1 KB | Medha Kumari |
| #40 | interdiff_39-40.txt | 414 bytes | immaculatexavier |
| #40 | 2399965-40.patch | 9.41 KB | immaculatexavier |
| |||
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedI should say that in an ideal world config entities wouldn't have get( ) and set() at all, and forms would mutate values by calling setters, like Symfony Form does, but it's too late for that now.
Comment #2
bojanz CreditAttribution: bojanz commentedInitial patch. Let's see what blows up (apart from the blocks which I fixed).
Comment #3
bojanz CreditAttribution: bojanz commentedComment #7
Mile23+1 on addressing this architectural question, in general.
-1 on punting to
get()andset(). Just remove them.This is all being refactored already in existing issues, under #2016679: Expand Entity Type interfaces to provide methods, protect the properties, so making a decision here about whether classes should add their own accessors, or just call
parent::set()/get()will make life a lot easier in those other issues.Also if keeping get/set is the plan, then we'll need to update the docblock for ConfigEntityBase to explain get and set, when and how to use them, what the API is.
Comment #8
alexpottGot the installer to work and some tests to pass - let's see what else breaks.
Comment #10
alexpottI expected more fails but the ConfigImportAll test is pretty extensive :)
Comment #11
bojanz CreditAttribution: bojanz commentedI fully support the approach in #10.
We've long ago decided to mandate getters and setters for config entities. Avoiding them via get()/set() is a loophole you can drive a truck through, big enough to make any logic in getters and setters useless. So there's a big potential data consistency issue here which can lead to bugs.
However, there's no way we can kill get() / set() for 8.0. So, we do the next best thing, we make them call the getters and setters.
The patch also has a number of fixes where various getters and setters were calling get() / set() themselves, for no good reason.
$excludedGetters is a bit ugly, but it clearly documents that we have several getters that are currently architecturally wrong, and which do weird things.
We might fix some before 8.0, or (more likely) we might fix them all for 8.1.x. It's still good to show clearly that they need future attention.
So, all in all, we've closed the hole, we've improved core consistency, we've left the room for future cleanups. Good enough for me.
Comment #12
tim.plunkettHm, This should have failed. Adding tests for it.
Comment #13
Mile23Comment #14
Mile23Great. Where's the @deprecated annotation, or at least the 'This is an anti-pattern and is bad' comment?
Comment #15
tim.plunkettI think this is fine for 8.1.x, but too late for 8.0.x IMO
Comment #28
dwwThis came up as the daily #bugsmash target today. A couple of us read the title and summary and said "sounds like a task, not a bug". I closely read the whole issue and saw that's how @bojanz submitted it. It was moved to a bug in #7 without any real explanation, other than tagging for DX. Although the change will probably help prevent other bugs, this issue isn't itself fixing a bug. 😉 It's proposing we change our APIs to harden them up, which is still a task. Doesn't mean it's not important. And hopefully this random attention will breath new life into this and help get it actually done. Updating the tags to be more accurate (definitely no longer relevant to any beta evaluation, but needs a reroll).
Thanks,
-Derek
Comment #29
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 9.4.x. Please review
Comment #30
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedTried to fix the custom command failure.
Comment #32
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe patch #30 does not apply
Comment #33
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe status is changed to Need Work.
Comment #34
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe status is changed to Need Work.
Comment #35
RassoniComment #36
RassoniTried to fix the patch failing issue.
Comment #37
RassoniComment #38
RassoniComment #39
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed the custom commands against #36
Comment #40
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed the custom commands against #40
Comment #42
Medha KumariReroll the patch #40 with 10.1.x .
Comment #43
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.