@godotislate

@karenann see 🐛 APCu requirement for 32MB always displays since APCu 5.1.25 Active . It should be fixed in the next patch release for 10.5+. 10.4 is on security support only, but you can see if the commit for 10.5 applies as a patch to 10.4.

Merge conflict because 📌 Slowly, very slowly start OOPifying the render system Needs review was reverted, but maybe just gonna wait to see if it gets back in soon.

Since 📌 Slowly, very slowly start OOPifying the render system Needs review has been reverted for now, postponing.

Wanted to see what this one needs next?

I resolved MR threads and rebase to get the tests to run again. I think it's ready.

Couple more thoughts:

Another alternative is to stop using references as I mentioned in #26. Every array would just be copied into and out of the element objects, and there would be no ##object properties. This would mean the element objects would need to be instantiated new every time you wanted to convert an array into one. I also have slight concerns wrt memory usage when copying what could be large render arrays back and forth.

Also, at some point in the future when the arrays are completely in the past and we're only using objects, we'll have to get used to using shallow vs deep copies, so if there's some solution for that we can come up with now, all the better.

Is there a way to trigger an error on a shallow copy and require a deep copy?

No, it's just PHP assignment $array1 = $array2; and there's no way to hook into that.

People are used to being able to just duplicate and I think it's clear we're breaking that.

Agreed, but there aren't too many options. We could revert it all for now, and make it a major-only change(?), but that might not be better.

On one hand, it's not all render arrays that are affected, just ones built from element objects. On the other hand, the relevant arrays are content entity forms with field widgets.

Maybe duplicate is a better verb for the method?

I'm not opinionated on the terminology, but NestedArray has the mergeDeep() and mergeDeepArray() methods which is where I got it from.

Updated 12899 with an additional test for shallow vs deep copies of these render arrays, as well as element object cloning. I think the tests show that we probably can't avoid adding a recursive copy method, though I don't love this. While people aren't using the element objects to create render arrays yet, when they do, having copies of render arrays entangled with each other because of references will probably lead a lot of unexpected behavior.

Alternatively we'd probably need to avoid using references in methods like initializeInternalStorage() and addChild(), but doing that probably causes problems of its own.

Will add a CR for the deep copy if we're good with the 12899 approach.

dww → credited godotislate → .

Since 🐛 APCu requirement for 32MB always displays since APCu 5.1.25 Active was filed first, I guess we could close this one as fixed (with the stopgap) and copy the open MR here over there. Or close that one as a duplicate.

Those other list builders can be fixed in follow ups

Fair enough. Created 🐛 Capture access cacheability in getDefaultOption() methods for all relevant entity list builders Active . This otherwise lgtm.

godotislate → created an issue.

There was test failure, so I rebased. Test pass now, so this LGTM.

I posted. suggestions on the MR to use class_exists per #52.

Agreed with #19: if a filter format is improperly deleted somehow, this should not result in content data loss. If, for example, the project has the filter format configuration in version control, the format could be restored somewhat easily. But if all the data is gone, the restoration effort would be more difficult and would require database backups to be available.

What are next steps? Should this be submitted for usability review?

Back to NW to look at whether cacheability needs to be handled in below bc of access checking per #160.

core/modules/field_ui/src/FieldConfigListBuilder.php
core/modules/taxonomy/src/VocabularyListBuilder.php
core/modules/workspaces/src/WorkspaceListBuilder.php

Also, to add the comment out parameter to all getDefaultOptions() implementations per #158.

Pushed a commit addressing #64 with a test and also the MR comment about type declarations.

I think 🐛 Maxlength validation counts newlines twice Needs work can be closed as a duplicate now and credit ported here.

Ready for review again.

@libbna sounds like you can go ahead and change all those methods to protected per #10.

I push up a commit to MR 12899. Looks like @nicxvan and I ended up in similar places.

Tests pass now, but I think there might need to be more test coverage per #18, and probably a CR. Will think about that later because looking at all this have given me a headache for now.

One question: since it wasn't about the `##object` values of each element but about certain form elements being references and therefore altered as memory addresses at their source by other code.. don't we actually have a different root cause of now altering more than we altered before?
The ##object properties don't affect anything per se here, but we need to unset them because they are referencing the wrong place and could conceivably present problems in other ways.

When we use the element objects to build a render array , it can ends up something looking like this:

array $build = [
  #type => some type,
  ##object => Element object 1,
  child => ref [
    #type => another type
    ##object => Element object 2,
    ...
  ]
  ...
]

When the array is copied to a new variable $copy because of the references, it looks the same

array $copy = [
  #type => some type,
  ##object => Element object 1,
  child => ref [
    #type => another type
    ##object => Element object 2,
    ...
  ]
  ...
]

So we want to break those array references and object references, because without doing that, if you called ElementInfoManager::fromRenderable($copy['child']), this would affect Element object 2, and in turn, possibly the original $build array as well. Will probably need test coverage for this...

array $copy = [
  #type => some type,
  child => (not a reference) [
    #type => another type
    ...
  ]
  ...
]

This probably also means we need to keep an eye on anywhere in code where render arrays are copied...

OK, so what I think is special about the LMS form is that it is opened in a modal via AJAX POST request, and the initial build of the form is cached. For most forms, the initial build is on a GET request that can not be cached. This causes the issue with the unprocessed form being cached incorrectly.

I remembered I wrote a kernel test for 🐛 Form state storage changes in #process callbacks are not cached during form rebuild. Needs work that also addresses weird form caching errors, so I borrowed some of that to write a kernel test that I believe simulates what's going on and makes it easier to debug. I pushed up a commit with the new kernel test, and you can see that the test only job fails, but the "fix" with serializing the unprocessed form makes the test pass.

Given that it's somewhat unusual for the initial build of a form to be on a POST request, I think the priority can be bumped down to Major, but leaving it for now.

godotislate → changed the visibility of the branch 3177977-form-state-storage-test-only-test to hidden.

We need a helper on element info manager to copy the object correctly for this situation.
I'd tried something along the lines of $unprocessed_form = (clone $this->elementInfo->fromRenderable($form))->toRenderable();, and that didn't work, since I think the copy is too shallow.

Then I tried implementing __clone in RenderElementBase to recursively clone all the element objects, and I think I hit a stack overflow.

I also tried NestedArray::filter($form, static fn ($element) => !($element instanceof ElementInterface), and that OOM'd.

I just pushed up a commit to handle the serialize exception. This change makes the LMS test pass, as least locally, but it's not passing here.

I think the next step is to try to create a failing core test. LMS is interesting in that it has AJAX modals stacked on top of each other in the entity forms, so maybe there's something happening because of that that's not covered in even core media library tests. I haven't tracked exactly how/why the unprocessed form array is coupled to the original array. I just saw in the debugger that properties had changed after the call to doBuildForm($form), and hopefully a (simpler) core test can make it easier to see why that's happening.

Root cause is that in FormBuilder::processForm(), $form is copied to $unprocessed_form before the call to buildForm() maps user input to #value properties. The value of $unprocessed_form is what is cached at the end of processForm():

    if (!$form_state->isRebuilding() && $form_state->isCached()) {
      $this->setCache($form['#build_id'], $unprocessed_form, $form_state);
    }

So when the form is retrieved from cache, such as in an AJAX submit, the form element should not have #value set, so in handleInputElement(), (!isset($element['#value']) && !array_key_exists('#value', $element)) evaluates to TRUE, and $element['#value'] can be set to the input value.

With the change to ElementInterface objects, after $form is copied to $unprocessed_form, elements between the two stay coupled, because of use of objects and references to arrays within the render array. So before $unprocessed_form is saved to cache, elements within the array have been changed in sync with $form, and $element['#value'] throughout elements in the form is set. Then when the form is retrieved from cache, the user input can not populate $element['#value'], since it is already set.

I've tried a few different methods to decouple $unprocessed_form from $form, including just serializing the form (see MR 12899), but nothing's worked so far.

godotislate → made their first commit to this issue’s fork.

OK, used a ternary to set fallback multiplier of 1 if ini_get('apc.shm_segments') is empty. This passes tests in MR 12895.

OK so apc.shm_segments has apparently been removed:
https://git.drupalcode.org/project/drupalci_environments/-/blob/dev/php/...
https://pecl.php.net/package/APCu/5.1.25

Opened MR to check what the APC values were per #6, and it looks like `ini_get('apc.shm_segments')` might be empty, which would certainly be a problem.
https://git.drupalcode.org/issue/drupal-3539331/-/jobs/6083955

Let's do this and unblock some builds.

@catch also suggested in slack that maybe we disable Status test for now to get builds working, then reinstate after finding the root cause.

It might be good to print what ini_get('apc.shm_size') and ini_get('apc.shm_segments') actually are now in the build env, and whether something isn't converting those values right in Bytes::toNumber.

godotislate → created an issue.

Following up from 🐛 Can't update to 11.2.0 Active here: with PHP10 support removed, consider restoring the core-recommended composer.json constraint on sebastian/diff.

Removed sebastian/diff from core-recommended/composer.json. MR ready.

or allow 4/5/6/7 in 11.2 with a release note, but open a new issue to bump to 5 in 11.3.

Conceivably we could do the latter in this issue, with a backport to 11.2 without the bump to 5? I did not do this in the MR, but I assume it's straightforward to do.

I wonder that since these methods are public now, we can't just change them to protected right away, but need to trigger deprecations and add a CR that they will be changed to protected in D12.

Might be worth a follow-up to try to remove the overrides, then we wouldn't need to update them in Drupal 12 except for when we delete them.

There are a lot of them:

core/modules/block/src/BlockListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/block_content/src/BlockContentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/comment/src/CommentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/field_ui/src/FieldConfigListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/filter/src/FilterFormatListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/image/src/ImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_ui/src/MenuListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/node/src/NodeTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/responsive_image/src/ResponsiveImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/search/src/SearchPageListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/shortcut/src/ShortcutSetListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/taxonomy/src/VocabularyListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/user/src/RoleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/views_ui/src/ViewListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

Two content entity list builders too:

core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

They do various things, so I don't think they can be removed.

Of relevance to this issue, these implementations of getDefaultOperations() include access checks inside them, so I wonder if cacheability needs to be captured.
core/modules/field_ui/src/FieldConfigListBuilder.php
core/modules/taxonomy/src/VocabularyListBuilder.php
core/modules/workspaces/src/WorkspaceListBuilder.php

Oh, and a couple content entity list builders override as public as well:

core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

core/modules/block/src/BlockListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/block_content/src/BlockContentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/comment/src/CommentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/field_ui/src/FieldConfigListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/filter/src/FilterFormatListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/image/src/ImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_ui/src/MenuListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/node/src/NodeTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/responsive_image/src/ResponsiveImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/search/src/SearchPageListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/shortcut/src/ShortcutSetListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/taxonomy/src/VocabularyListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/user/src/RoleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/views_ui/src/ViewListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

There are a couple test failures, not sure if intermittent or unrelated.

One thing to think about, and maybe this is a follow up:
Deprecate prepending the decorated http_kernel.basic service object directly, so that it's always the service closure prepended as an argument to each http middleware instead. That way we eventually don't need to run reflection and get the constructor arguments at all.

DrupalCoreRecommendedBuilder creates composer/Metapackage/CoreRecommended/composer.json based on the root composer.lock, so to loosening the constraint on sebastian/diff in composer/Metapackage/CoreRecommended/composer.json has to be done manually. However, that makes it fail the integrity test in Drupal\Tests\Composer\Generator\MetapackageUpdateTest.

One solution could be to add sebastian/diff to $remove_list in DrupalCoreRecommendedBuilder::getPackage(), so that it doesn't get added to composer/Metapackage/CoreRecommended/composer.json, which would relax it to the version constraint in core/composer.json is "sebastian/diff": "^4 || ^5 || ^6 || ^7". Though this seems pretty broad?

godotislate → changed the visibility of the branch 3531287-loosen-sebastiandiff-constraint to hidden.

godotislate → made their first commit to this issue’s fork.

A few small comments, mostly about typing.

A couple questions on the MR.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

Random thought would be to do some sort of check in the status report, though the detection would likely be limited to globally placed menu blocks, and maaaaybe menu blocks in LB defaults. But then being able to set the config property would require some pretty technical knowledge, whether setting via drush or the config sync UI or whatever.

I guess the question here is, given that filter formats are not intended to be deleted, what handling should there be in the case that a drush command, or contrib/custom code was not sufficiently careful to prevent deletion?

I'm not sure what the best path forward is.

After making the switch in the UI from essentially "ignore active trail" to "show active trail", if we flip the config property to match, then we end in the position of needing to update existing block configurations and how to deal with them in layout builder. We could just update global block configs, leave LB blocks unchanged, and then in code treat the "show_active_trail" property being unset to be the same as TRUE.

Alternatively, we could leave the property as "ignore_active_trail", and then in the block config form submit, flip the form value* before saving to config. I don't know if that's done anywhere else though, and it seems counterintuitive and bad DX.

*I think there's also some consideration needed on how handle the form value because the checkbox can be disabled by states, though maybe the easiest thing would be not to disable the field and only set visible/invisible.

And I think that might be fine for a Drupal 12 change because that code can be rewritten to use ::hasField(), then the actual exception can be deprecated for removal in Drupal 13.

Maybe I'm misunderstanding, but it's throwing \InvalidArgumentException, so there's no deprecation or removal needed for the exception itself.

We can change the code in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider to use hasField instead of exception handling though.

Well, that is then in many cases traded into php warnings (accessing property on null) or fatal errors ($entity->get('field')->anyMethodCall()).

Yeah, my thought was incomplete. But basically I'd find it more convenient guarding against that type of error with a nullsafe operator.

I'm confused why MR !12845 doesn't need the change in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider. That should trigger the deprecation now, no?

The exception still needs to be handled until we stop throwing it (in a follow up presumably), so that's why there's no change in that class. And apparently there's no test code that would throw the exception there, so no deprecation message emitted.

I'm not very fond of that approach without a way to explicitly disable the exception now. It makes it impossible to actually do if ($entity->get('field_name')?->value) until we are on D12/13 and remove the exception.

I agree. I can look into your suggestion in #50 instead, maybe next week.

OK, put up MR 12845 with the approach to just deprecate throwing the exception. Will update the CR and follow up issue to match if this turns out to be the preferred way to go.

It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with.

I've been on a project or two where fields are removed from entities, but devs miss removing code like $entity->get({REMOVED FIELD NAME}). Without adequate test coverage, it goes to prod and results in a WSOD.

This could also affect Twig templates and if and when the entity magic getter is removed and replaced with get() instead.

@xjm I think I got confused in #11.

Looking at the code again now, it doesn't look like calling $entity->delete() invokes entity access. I believe the access check needs to happen before delete() call. Once delete() is called, I don't think there's any way to stop that except by throwing an exception.

So FilterFormatAccessControlHandler::checkAccess() has this:

    // We do not allow filter formats to be deleted through the UI, because that
    // would render any content that uses them unusable.
    if ($operation == 'delete') {
      return AccessResult::forbidden();
    }

The reproduction steps in the IS use drush \Drupal\filter\Entity\FilterFormat::load('restricted_html')->delete();, which I guess bypasses checkAccess somehow. I haven't looked into why, and there's a note in the comment about deletion via UI and not CLI.

Regardless, maybe a better approach is explore denying filter format entity delete access via CLI like this as well?

Some minor nits and 1 question on the MR.

I kind of thought we already do... Would need to look properly at what's going on in config entity loading.

Right now, Drupal\Core\Entity\Attribute\ConfigEntityType has $static_cache default to FALSE,
and the only config entity types that have static_cache: TRUE are role and language_content_settings.

I'm not sure about the history about that or any complexities associated with enabling the static cache on more config entity types. Maybe @berdir would know?

1 question on the MR.

I wondered about somehow deprecating the exception itself, so that if it's thrown and caught, it would trigger the deprecation notice indicating that code should use hasField(), but I can't remember an equivalent problem and didn't think it all the way through yet.

Lack of precedent aside, I think this is an interesting idea. Though if we're still deprecating for Drupal 13, then we'd still have to do exception handling until then, which is a bit longer than I would hope.

Note that set() calls get(), and in previous comments we decided that set() shouldn't fail silently on unknown fields.

We can do the hasField() check within set() and throw the exception from there, though.

My suggestion to implement onDependencyRemoval() in #2 was in the wrong place. TextItemBase makes more sense on where to implement it, and that's what is in MR 12823.

This prevents the data from deleted, but there is a display issue, in that when the format is deleted, existing content with text fields using that filter format can not display the content. However, this can be addressed by editing the entity and setting the filter format to something else.

Leaving at NW for tests.

Looks like my proposed resolution in #16 overlaps a lot with 🐛 Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed Needs work and might make this a duplicate of that.

I think this can be done by implementing onDependencyRemoval(array $dependencies) in Drupal\field\Entity\FieldStorageConfig.

Basically, something like:

  public function onDependencyRemoval(array $dependencies) {
    $changed = parent::onDependencyRemoval($dependencies);
    if ($changed) {
      return TRUE;
    }

    if (!isset($dependencies['config'])) {
      return FALSE;
    }
    
    foreach ($dependencies['config'] as $config) {
       // If $config is a filter format, return TRUE.
    }
  }

Thanks @godotislate :)

You're welcome!

Rebased by request, so that the tests could run. There's a test failure, and not one I've seen before, so I'm not sure whether it's related to this MR or an intermittent failure.

godotislate → made their first commit to this issue’s fork.

1 small comment on the MR.

Also, @alexpott brought up in 🐛 Maxlength validation counts newlines twice Needs work #27 that normalizing "\r\n" to "\n" will cause values to be stored differently between form and JSON:API submissions when textareas are used in entity form widgets. Should core widgets set their textarea's #normalize_newlines property to FALSE?

@mherchel mentioned this in the original issue 📌 Update prettier/PostCSS/stylelint for 11.2 Active for the next step after this:

After that, we should really see what's holding us back from shipping native nesting. A quick look at https://caniuse.com/css-nesting, shows that our supported browsers support it.

I suppose a follow up is needed for that, but I don't know what needs to be done there.

This and 📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active have also been identified as blockers for ✨ Support hooks (Hook attribute) in components Active , which would be a big win for testing and also fixes current issues with decorating and altering hook services on the side.

Since we've officially deferred disruptive deprecations, 10.6.x is more likely to be API-compatible with 11.2.x than 10.5.x was with 11.1.x, so I agree that release notes are probably sufficient for the next minor.

Can I suggest that the update hook not be skipped for 10.6.x? At the very least, to prevent someone from going from 10.6.x to 11.1.x, however unlikely that might be? Especially since 10.5/11.2 have the breaking CKEditor5 changes.

One tiny comment on the MR.

Tagging "Needs change record" to document the use of CacheOptionalInterface when implementing block plugins. Along the lines of MR comment from catch:

Agreed that documenting this is a good idea, but yeah the idea here is that:

• cache optional and placeholdered blocks are still cached by the internal page cache or proxies (so not max-age 0), but not anywhere else.
• cache optional blocks that aren't placeholders are still cached by dynamic page cache, just not individually - when we think the cost of rendering them directly on a dynamic render cache miss might be less than cache retrieval.

OK, tested locally, LGTM.

11.x

SIMPLETEST_DB="sqlite://localhost/sites/default/files/db.sqlite" php -d memory_limit=1G core/scripts/run-tests.sh --verbose --sqlite test.sqlite --color --types PHPUnit-Unit Lock

Drupal test run

Drupal Version:   11.3-dev
PHP Version:      8.3.21
PHP Binary:       /usr/bin/php
PHPUnit Version:  11.5.22
-------------------------------

Tests to be run:
  - Drupal\Tests\Core\Lock\LockBackendAbstractTest

Test run started:
  Tuesday, July 15, 2025 - 09:30

Test summary
------------

   2.180s Drupal\Tests\Core\Lock\LockBackendAbstractTest                            3 passed, 1 log(s)

Test run duration: 1 sec

Detailed test results
---------------------


---- Drupal\Tests\Core\Lock\LockBackendAbstractTest ----


Status      Duration Info
--------------------------------------------------------------------------------------------------------
Pass          0.033s testWaitFalse
Pass          1.007s testWaitTrue
Pass          0.002s testGetLockId
Log           1.139s *** Process execution output ***
    PHPUnit 11.5.22 by Sebastian Bergmann and contributors.

    Runtime:       PHP 8.3.21
    Configuration: /var/www/html/core/phpunit.xml

    ...                                                                 3 / 3 (100%)

    Time: 00:01.044, Memory: 6.00 MB

    Lock Backend Abstract (Drupal\Tests\Core\Lock\LockBackendAbstract)
     ✔ Wait false
     ✔ Wait true
     ✔ Get lock id

After

SIMPLETEST_DB="sqlite://localhost/sites/default/files/db.sqlite" php -d memory_limit=1G core/scripts/run-tests.sh --verbose --sqlite test.sqlite --color --types PHPUnit-Unit Lock

Drupal test run

Drupal Version:   11.3-dev
PHP Version:      8.3.21
PHP Binary:       /usr/bin/php
PHPUnit Version:  11.5.22
-------------------------------

Tests to be run:
  - Drupal\Tests\Core\Lock\LockBackendAbstractTest

Test run started:
  Tuesday, July 15, 2025 - 09:29

Test summary
------------

   2.195s Drupal\Tests\Core\Lock\LockBackendAbstractTest                            3 passed, exit code 1

Test run duration: 1 sec

Detailed test results
---------------------


---- Drupal\Tests\Core\Lock\LockBackendAbstractTest ----


Status      Duration Info
--------------------------------------------------------------------------------------------------------
Pass          0.033s testWaitFalse
Pass          1.008s testWaitTrue
Pass          0.002s testGetLockId
Failure       1.152s *** Process execution output ***
    PHPUnit 11.5.22 by Sebastian Bergmann and contributors.

    Runtime:       PHP 8.3.21
    Configuration: /var/www/html/core/phpunit.xml

    ...                                                                 3 / 3 (100%)

    Time: 00:01.046, Memory: 6.00 MB

    Lock Backend Abstract (Drupal\Tests\Core\Lock\LockBackendAbstract)
     ✔ Wait false
     ✔ Wait true
     ✔ Get lock id

Haven't had a chance yet to test locally what the junit looks like, but had a question about the need for and placement of the update hook.

A couple comments on the MR.
CR also needs a version bump.

Added a couple comments to MR, nothing major I think. Nice work!

Addressed the outstanding MR comments. I think the cache contexts for the LanguageBlock might not be as complex as thought, unless I'm missing something, but I also think it's fine to handle in a follow up if not adequately addressed.

Thanks, @alexpott. The way I tried testing before was:

1. install umami
2. export config
3. copy views config yml to a separate folder
4. install umami again
5. drush cim --partial --source={folder with views files exported before}

Which wasn't showing a lot of difference in memory usage.

But now, testing with:

1. install umami
2. export config
3. Run script to change views config per #8
4. drush cim

I'm seeing:
Before
[notice] Synchronized configuration: update views.view.archive. [1.3 sec, 26.3 MB]
[notice] Synchronized configuration: update views.view.articles_aside. [1.3 sec, 27.16 MB]
[notice] Synchronized configuration: update views.view.block_content. [1.31 sec, 28.65 MB]
[notice] Synchronized configuration: update views.view.content. [1.33 sec, 29.9 MB]
[notice] Synchronized configuration: update views.view.content_recent. [1.34 sec, 30.83 MB]
[notice] Synchronized configuration: update views.view.featured_articles. [1.35 sec, 30.05 MB]
[notice] Synchronized configuration: update views.view.files. [1.37 sec, 32.6 MB]
[notice] Synchronized configuration: update views.view.frontpage. [1.38 sec, 33.78 MB]
[notice] Synchronized configuration: update views.view.glossary. [1.39 sec, 33.74 MB]
[notice] Synchronized configuration: update views.view.media. [1.41 sec, 35.61 MB]
[notice] Synchronized configuration: update views.view.media_library. [1.44 sec, 38.83 MB]
[notice] Synchronized configuration: update views.view.moderated_content. [1.46 sec, 39.92 MB]
[notice] Synchronized configuration: update views.view.promoted_items. [1.48 sec, 39.83 MB]
[notice] Synchronized configuration: update views.view.recipe_collections. [1.48 sec, 40.52 MB]
[notice] Synchronized configuration: update views.view.recipes. [1.5 sec, 39.25 MB]
[notice] Synchronized configuration: update views.view.related_recipes. [1.51 sec, 40.2 MB]
[notice] Synchronized configuration: update views.view.taxonomy_term. [1.51 sec, 41.2 MB]
[notice] Synchronized configuration: update views.view.user_admin_people. [1.53 sec, 42.85 MB]
[notice] Synchronized configuration: update views.view.watchdog. [1.55 sec, 44.52 MB]
[notice] Synchronized configuration: update views.view.who_s_new. [1.55 sec, 45.15 MB]
[notice] Synchronized configuration: update views.view.who_s_online. [1.56 sec, 43.57 MB]
[notice] Finalizing configuration synchronization. [1.65 sec, 44.03 MB]
[success] The configuration was imported successfully. [1.66 sec, 44.03 MB]

After
[notice] Synchronized configuration: update views.view.archive. [1.07 sec, 26.37 MB]
[notice] Synchronized configuration: update views.view.articles_aside. [1.08 sec, 27.22 MB]
[notice] Synchronized configuration: update views.view.block_content. [1.09 sec, 28.71 MB]
[notice] Synchronized configuration: update views.view.content. [1.1 sec, 28.36 MB]
[notice] Synchronized configuration: update views.view.content_recent. [1.11 sec, 29.29 MB]
[notice] Synchronized configuration: update views.view.featured_articles. [1.12 sec, 27.03 MB]
[notice] Synchronized configuration: update views.view.files. [1.15 sec, 27.41 MB]
[notice] Synchronized configuration: update views.view.frontpage. [1.16 sec, 28.58 MB]
[notice] Synchronized configuration: update views.view.glossary. [1.17 sec, 27.96 MB]
[notice] Synchronized configuration: update views.view.media. [1.19 sec, 29.1 MB]
[notice] Synchronized configuration: update views.view.media_library. [1.22 sec, 31.11 MB]
[notice] Synchronized configuration: update views.view.moderated_content. [1.24 sec, 30.25 MB]
[notice] Synchronized configuration: update views.view.promoted_items. [1.25 sec, 29.06 MB]
[notice] Synchronized configuration: update views.view.recipe_collections. [1.26 sec, 29.74 MB]
[notice] Synchronized configuration: update views.view.recipes. [1.27 sec, 26.86 MB]
[notice] Synchronized configuration: update views.view.related_recipes. [1.28 sec, 27.81 MB]
[notice] Synchronized configuration: update views.view.taxonomy_term. [1.28 sec, 28.81 MB]
[notice] Synchronized configuration: update views.view.user_admin_people. [1.3 sec, 29.49 MB]
[notice] Synchronized configuration: update views.view.watchdog. [1.32 sec, 29.67 MB]
[notice] Synchronized configuration: update views.view.who_s_new. [1.32 sec, 30.3 MB]
[notice] Synchronized configuration: update views.view.who_s_online. [1.33 sec, 27.11 MB]
[notice] Finalizing configuration synchronization. [1.36 sec, 27.57 MB]
[success] The configuration was imported successfully. [1.36 sec, 27.57 MB]

Seeing a significant improvement! Moving to RTBC.

I don't have a test site with a ton of views, or at least, enough views, to validate the memory usage reduction. I tried installing umami, tweaking the views there, and running config import, but the numbers were not noticeably different.

That said, these changes make sense to me, and I validated that the schemaWrapper does not get used outside of save()/castValue() and is safe to set to NULL.

RTBC +1. Holding off moving in case someone wants to validate the memory leak fix.

OK, I think the MR is ready for now.

A couple notes:

• I didn't change any of the protected methods in Renderer, to keep the scope of the changes down. The render objects are converted back to arrays before the calls to the protected methods
• Not sure if the type declaration announcement changes are done completely or if there are more steps to do the deprecations
• Also not sure about how to do the change record. Should https://www.drupal.org/node/3534020 → be reused and updated? But then, we can't make changes to it until this issue is merged? How do we handle superseding changes to the same method signature?
• TIL (well, yesterday, technically) ternaries and arrow functions don't really support reference variables

Re-based to resolve merge conflict. Since this applying the new nesting format on top of changed CSS from upstream, moving to NR first. Though ideally this is either merged somewhat soon or decided not to be done, because every CSS change upstream will likely lead to a merge conflict.

Side note, I find generated selectors like this pretty amusing: :is(:is(:is(body:not(.is-always-mobile-nav) .primary-nav__menu-link--level-2) .primary-nav__menu-link-inner)::after):dir(rtl)

Got the MR started with an updated test. Surprisingly everything passes.
Leaving at NR for some clean up.

In this context, deprecation means notifying that the union type declaration is coming in D12, not that arrays will be removed.

Created follow-up per #33: 📌 [PP-1]Allow RenderableInterface objects to be passed as the $elements parameter in RendererInterface::render* methods Active .

Is there a meta issue about moving to using render elements in the Render API?

godotislate → created an issue.

@penyaskito: the parameter and typehint already exist in the docblock, so there's nothing to add there, AFAICT. Not sure how that affects whatever triggers the deprecation warning described in the docs, though.

If we exclude adding the identifier as a parameter then it has to refer to the method it is in for all hooks using that method.

If the code inside the method requires modules to be installed, then I'm not sure what difference it makes which hook(s) the method is implementing. If somehow the method has conditionals where it can run without modules in a certain context/hook, but not without in others, either the code should be separated into multiple methods, or the the attribute should not be added at all.

I think the attribute should work pretty straightforwardly: if the attribute is on a method, then if the required modules are not installed, then the method is not registered as an implementation for any hook.

If the attribute is on a class (up for discussion whether this should be supported), then no method in that class should be registered as an implementation for any hook if the required modules are not installed.

Are there any use cases that this wouldn't work for?

Oh, nice! I'd forgotten about that.

I think the new name is OK if we want to be flexible for possible new use cases. Or at least, I don't have a better suggestion.

That said, I keeping this simple might be best. Allowing for callables introduces the possibility of some developer trying to do something overly complex in their project and using code from other modules in the callable. Or someone will try to instantiate a service in the callable, even though the attribute is parsed at compile time and doing that is less than ideal.

As it is, implementing the attribute should be careful not to instantiate the module handler to check for modules being installed and try checking against the container.namespaces parameter instead. Example of that being done is in 📌 Add a fallback classloader that can handle missing traits Active .

If it's helpful for consideration in merging, just wanted to mention again (as in #8) that the diff may make the changes look more significant than they are. There's very little actual new code in the MR. Mostly things are just moved around to consolidate in one place in the submodule.

I tried to apply all the changes from the merge request, but I wasn’t able to because some had already been merged while others hadn’t.

I believe the MR diff should apply clean to 3.0.4., if you're willing to update.

Can I suggest we call the attribute something like "DependsOn"?

I actually proposed a Dependencies attribute in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active to solve a similar issue for plugin discovery (with a bigger problem there being that the attribute couldn't be parsed by Reflection when necessary modules aren't installed), so I think either of those is more readable than HookBy.

I think it also makes sense for the attribute to take an array of modules as the parameter, or a string|array union, just in case there are multiple modules that need to be installed for the hook.

@gábor hojtsy: I see now that the entity_builder is supposed to populate the third party setting on the add form, before going to the edit form in the test.

The issue about the edit form field not being populated correctly isn't an issue with the entity builder not being invoked, it's with this logic in PluralFormulaLanguageFormBase::form():

// Pick previous plural formula if editing an existing language.
    $plural_formula = '';
    if (isset($form['langcode']['#value'])) {
      $editingLangcode = $form['langcode']['#value'];
      /** @var \Drupal\language\ConfigurableLanguageInterface $language */
      $language = $form_state->getFormObject()->getEntity();
      $plural_formula = $language->getThirdPartySetting('l10n_pconfig', 'formula');
    }

In Drupal\language\Form\LanguageFormBase::commonForm(), this is how the form is built:

if ($language->getId()) {
      $form['langcode_view'] = [
        '#type' => 'item',
        '#title' => $this->t('Language code'),
        '#markup' => $language->id(),
      ];
    }
    else {
      $form['langcode'] = [
        '#type' => 'textfield',
        '#title' => $this->t('Language code'),
        '#maxlength' => 12,
        '#required' => TRUE,
        '#default_value' => '',
        '#disabled' => FALSE,
        '#description' => $this->t('Use language codes as <a href=":w3ctags">defined by the W3C</a> for interoperability. <em>Examples: "en", "en-gb" and "zh-hant".</em>', [':w3ctags' => 'https://www.w3.org/International/articles/language-tags/']),
      ];
    }

So isset($form['langcode']['#value']) in PluralFormulaLanguageFormBase::form() is FALSE, and the third party settings are not loaded to populate the form value.

I think open MR comments have been addressed.