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.PluginManagerBase::processDefinition() helpfully merges in default values, but it does so with +=, which only works for one dimensional arrays.
NestedArray (the OO version of drupal_merge_deep_array()) provides a way to recursively adde default values, without altering the original values.
It is also a Component, so there is not too much pollution there.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | drupal-1816916-9.patch | 7.14 KB | tim.plunkett |
| #9 | interdiff.txt | 7.43 KB | tim.plunkett |
| #2 | drupal-1816916-2-FAIL.patch | 2.74 KB | tim.plunkett |
| #2 | drupal-1816916-2-PASS.patch | 3.42 KB | tim.plunkett |
| #1 | drupal-1816916-1.patch | 697 bytes | tim.plunkett |
Comments
Comment #1
tim.plunkettThis is needed for #1763974: Convert entity type info into plugins, and I can see it preventing some major WTF moments later.
Comment #2
tim.plunkettThis shows the problem, as well as echoing #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator by showing the test was wrong all along.
Comment #3
neclimdulSeems straight forward enough. I think I was just lazy and going for what worked quickly.
I don't know if we require tests for simple features like this but since it doesn't regress, change any documented functionality, or fix a bug I don't see the need. It looks good from my perspective.
Comment #4
neclimdulx-post.
With tests! Even better. Assuming it passes my RTBC stands.
Comment #5
tstoecklerYup, makes total sense. I was about to propose the same in #1763974: Convert entity type info into plugins, but splitting this off is even better.
Comment #6
webchickHm. Could we get a separate test about this nested array thing specifically? The tests we're modifying are "// A simple plugin: a mock user login block." which are a good starting point for people to understand the plugin system, and mysteriously adding a weird "child" index to the user login form that doesn't actually exist seems to make that more confusing.
Comment #7
tstoecklerHaving recently written some tests for the plugin discovery system, I can assure you that that, sadly, is not quite the case currently. The whole Plugin test suite could use some serious refactoring for code coverage, decoupling and understandability. That said, you are 100% correct, that we shouldn't make things worse. Unless someone beats me to it, I'll cook up some tests this weekend.
Since the discovery tests are separated from the rest now, I think it would be easier to write tests for #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator, which contains this issue, and mark this duplicate. What do you
guysfolks think about that? I think that would make sense, as that issue is where we want to go anyway with this.EDIT: Less sexist form of address :-), hoping no toes were stepped upon.
Comment #8
tim.plunkett#6, yes I can write a separate test
#7, the approach in that issue is flawed, and harms DX to boot. I'll follow up over there after I reroll this.
Comment #9
tim.plunkettThe interdiff is pretty uninformative. But this adds a new manager, a new plugin, and new assertions.
Comment #10
tim.plunkettI give up, #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator is going to have to be the solution here.
Comment #11
tim.plunkettAfter more discussion with @neclimdul, I realized my problems with this were because I was using the cache incorrectly. This is a correct and non-controversial fix, and the concept of a DefaultsDecorator can continue in the other issue.
Comment #12
neclimdulComment #13
neclimdulerr, forgot to comment. This is still straight forward and we've got more tests now! RTBC!
Comment #14
tim.plunkettThis blocks #1763974: Convert entity type info into plugins which is major, so bumping to major and postponing that.
Comment #15
sunLooks good to me as well.
Comment #16
webchickWow, that's even better than I was expecting. Thanks!
Committed and pushed to 8.x.