@godotislate

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.

Oops, well the change can be reverted if needed :)

AFAICT, entity builders are only invoked on a request with form input, not on the initial form build. Populating the field on the initial build probably needs the #default_value to be set or similar?

OK, rebased the MR and updated the deprecation version. I also changed the code a bit so that $variables['date'] and $variables['author_name'] are always set, even if empty, so that people debugging Twig templates don't wonder why those variable might be missing.

CR draft: https://www.drupal.org/node/3534020 →
Follow up to add the type declaration: 📌 [PP-1] Add array type declaration to parameters in RendererInterface::render() Active

One thing to consider is whether to add type declarations to $elements in renderRoot(), renderInIsolation(), and renderPlain() as well, but that's probably out of scope for this issue.

godotislate → created an issue. See original summary → .

Can you please advise me if you think I should refactor my test coverage 'borrowing' from your code to take account of new lines?

I don't think it's easily accomplishable in a unit test. As for the rest, I would just leave it as is.

Gating it behind fields with a maxlength is a great idea - nice one

Added this to the MR and additional tests. Ready for review again.

Let's just move it back to NW. Can be picked up if anyone expresses interest again.

OK, added more general tests. I also removed the NodeAccessCacheability kernel test I'd added before, because I think it's duplicating other tests mostly at this point, and instead added asserts in the functional test to test that the query tags get added to the render array when the query is not running in a render context.

Additionally, I found dead/broken test code #2878483: loadEntityByUuid() should skip access checks → in query alter hook in EntityTestHooks.php, and I removed all that and reused the hook for tests needed here.

One last thing:
It occurred to me that maybe entity query objects should include the corresponding entity type list cache tags and entity type list cache contexts. However, when I tried that the list cache contexts for the Node entity type are ['user.node_grants:view'], which would kinda defeat the point of all the logic in the query alter hook only to add those contexts conditionally.

Separately I was also wondering if the page cache contexts should be added to PagerSelectExtender, but I haven't thought that through completely.

I think the MR is ready for review regardless.

In \Drupal\Core\Render\Element\Textfield::valueCallback we have return str_replace(["\r", "\n"], '', $input);

That str_replace() strips out every instance of \r or \n from the input, so \r\n would be stripped out as well. Though technically, textfields and other <input> elements don't allow new lines in input: https://stackoverflow.com/a/42744064. That's a stackoverflow answer, but from a quick Google, official resources seem to say the same without being as declarative.

Doing some git archaeology, I found that that str_replace comes from way back in #53480: required checkbox not stylized like a required field → , and it was for form input hardening (see #6 → ):

We remove linebreaks from mere textfields, there is no way this breaks user data, only hackers are interested. This puts many header injection attacks at rest.

If we need to add similar hardening for non-form submissions to telephone, machine_name, etc., I think that'd be a separate issue?

Back to textareas, a couple thoughts:

I don't think there are any field widgets in core that set the #maxlength property on a Textarea form element object. So if we gate replacing "/\r\n?/" with "\n" based on whether #maxlength is set, something like this:
return isset($element['#maxlength']) ? preg_replace("/\r\n?/", "\n", (string) $input) : (string) $input;
then values submitted via JSON:API requests will be the same as content entity form submissions, except for contrib/custom field widgets that set #maxlength on the Textarea. Arguably it would be on the devs implementing the field widgets, and possibly their respective field types, to add the appropriate Constraints and normalize the JSON:API data.

An alternative idea is to add a kernel request event subscriber that goes through all properties in $request->query and $request->request, and request content as well for requests with JSON bodies, and replace all \r\n with \n. But doing this for request content is tricky, and I think this approach overall might be overly aggressive.

My question then is what is the plan and time line to remove these dependencies?

I haven't been following this issue closely, but speaking generally: often in situations like this, remaining work gets split off into follow ups. Reducing scope in individual issues makes the diffs smaller, easier to review, and more likely to be committed.

FWIW, since 📌 Pass RenderContext around in the Renderer Active is in, I applied the changes to core/lib/Drupal/Core/Entity/EntityStorageBase.php from MR 12444 to 11.x and still see unreplaced big pipe placeholders on the Umami home page, even with the exclusion of file entities. Of note is that one of the unreplaced placeholders is the recipe collection view block, which does not contain any images.

It could be that I didn't cherry-pick the changes correctly, but I don't think that's the case.

CR: https://www.drupal.org/node/3533514 →

@xjm The problem/motivation in ✨ Adopt phpstan phpdoc types as canonical reference of allowed types Active has "I propose that anything included in https://phpstan.org/writing-php-code/phpdoc-types should be allowed", but I agree that the updates made to the CS docs are not as definitive.

I bring it up because the use of array shapes and similar has been begun to be adopted in pending MRs across quite a few core issues, so if tools needing to be in place is a blocker, there might need to be more general awareness of that.

Seems like there are open questions on the MR.

There is also sadly one point for a code style and documentation quality reduction WRT the array PHPDoc hinting.

Coding standards were updated to allow all documented PHPStan PHPdoc types per https://www.drupal.org/node/3505429 → .

Is fixing api.d.o to work with these types correctly a blocker?

I ran these commands:
phpunit --list-tests --group package_manager | grep Package
phpunit --list-tests --group="#slow" | grep Package

on both this MR branch and 11.x and confirmed that the following all show up as expected:

 - Drupal\Tests\package_manager\Build\PackageInstallDirectWriteTest::testPackageInstall
 - Drupal\Tests\package_manager\Build\PackageInstallSubmoduleTest::testSubModules
 - Drupal\Tests\package_manager\Build\PackageInstallTest::testPackageInstall
 - Drupal\Tests\package_manager\Build\PackageUpdateTest::testPackageUpdate

Changes look straightforward. LGTM.

Put up alternate suggestion in MR 69.
This uses setter injection via method calls in the service definition to avoid overriding the constructor altogether.

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

Yeah, and env support has hit a snag in 📌 Support setting service parameters via environment variables Needs work .

Per @finnsky on Slack, the regression actually came from 🐛 Navigation top bar should utilize Drupal.dispace() Active .

Should this be moved to NW

IS probably needs to be updated per #106-108 for proposed resolution.

Tests passed.

Rebased, will move to NR once tests pass.

Not sure about need for the action to replace the original image file with one that has a style applied to it, but will leave that decision to a maintainer. But at the very least, I think it needs some sort of restricted permission associated with it, to be able to control who can overwrite files this way.

The Autowire attribute also can be passed container parameters, so those might be something to be accounted for by the trait as well.

Technically also expressions and env as well, but I don't think the Drupal container supports those atm.

Yes, it looks like Select is essentially a base class. SelectExtender also implements SelectInterface, but it is essentially a base class as well. And for entity queries, QueryBase is the base class for QueryInterface. Updated the MR to have the respective interfaces extend RefinableCacheableDependencyInterface and added a CR [#3533258].

The SQL entity query class merges the cache metadata from the underlying Select object, so this works for content entities. Config entity queries or other entity types using key value storage don't capture any cacheable metadata from SQL key value queries. Doing so would add complexity to key value and this seems like an edge case.

Follow up per #11: ✨ [PP-1] Make it possible to capture cacheable metadata from underlying entity queries in EntityStorageInterface::loadByProperties() Active .

Probably still needs some more test, so leaving in NW and the Needs tests tag.

godotislate → created an issue.

Should there be an optional second parameter on loadByProperties() to capture cacheable metadata?

Then there's the EntityRepository method loadEntityByUuid method that calls EntityStorage::loadByProperties, and possible other rabbit holes, so I don't know if it makes sense to add an additional parameter everywhere, at least not in scope for this issue.

Really? Yes it does.... I've confirmed that with a debugger. This is quite messed up right? What a PITA.

Yes, this was surprising to me as well, but apparently it is HTML spec. See #19 🐛 Maxlength validation counts newlines twice Needs work .

Sure you can enforce max length on the field level but backing a text area field with anything other than big text field feels like you are opening a can of worms.

I don't think I necessarily disagree, esp for sites with content translation, but is it obvious? #5 🐛 Maxlength validation counts newlines twice Needs work and #13 🐛 Maxlength validation counts newlines twice Needs work brought up the storage concerns.
Also, it's not uncommon for stakeholders to request to have title (replacement) fields that support formatted text and new lines. This could be implemented as a TextItem field, with an alter to allow a textarea widget to be used. But maybe it's too edge-casey?

Also I think that point 4 from #27 is important.

Agreed, not sure about what to do about that and open to ideas. Is maxlength checked at all for JSON:API requests, since the validation is done in FormValidator?

Is it really okay to change user input. Normally Drupal saves what the user enters and makes changes on output.

The reason to do that here is that it's possible that database storage has the same character limit, so without converting "\r\n" to "\n", it's possible there could be a database error if it passes JS and PHP validation. I don't think core has any instance of character-limited storage for a field that has textareas used for entry, but it seems possible for something in contrib or custom code.

Also, the browser is already converting "\n" or "\r" to "\r\n" on submit, so technically for users on non-Windows clients, the input they entered has already been changed.

I think I did try awhile ago to see if other web projects might be handling "\r\n" for similar concerns, but didn't find anything conclusive.

Put up draft MR 12535 implementing #6. Added a kernel test which essentially tests what Drupal\Tests\node\Functional\NodeAccessCacheabilityTest::testNodeAccessCacheabilitySafeguard() does, but without rendering. There probably should be additional test coverage for more generic usage.

A couple questions came to mind:

1. Should Drupal\Core\Database\Connection\SelectInterface and Drupal\Core\Entity\Query\QueryInterface extend RefinableCacheableDependencyInterface? It would eliminate the need to do instanceof checks, but there are BC considerations
2. Implementations of EntityStorageInterface::loadByProperties() use entity queries under the hood. Should there be an optional second parameter on loadByProperties() to capture cacheable metadata?

Does it makes for \Drupal\Core\Database\Query\Select (and maybe \Drupal\Core\Database\Query\SelectExtender) and \Drupal\Core\Entity\Query\QueryBase to implement \Drupal\Core\Cache\RefinableCacheableDependencyInterface and use \Drupal\Core\Cache\RefinableCacheableDependencyTrait? There's no getCacheableMetadata(), but there are getter methods for context, tags, and max-age, and you can also then merge the query object as a cacheable dependency to any other cacheable dependency object.

nicxvan → credited godotislate → .

I don't grok what is suggested in #98 sorry @godotislate

Last I checked, there's no schedule or announcement about when Twig 4 will be released, which is when spaceless will be removed from Twig.

If Twig 4 is released after Drupal 12, then Twig will still provide spaceless, so it won't really be removed in Drupal 12.

OK, finally got the test-only failing as expected: https://git.drupalcode.org/issue/drupal-3505049/-/jobs/5701154

The only reliable way I've found to reproduce the issue is to install the test container_initialize module, then run drush cr.

And the only way I've found to make a test do that is to install container_initialize, then make a request to core/rebuild.php, because both drush cr and core/rebuild.php make calls to drupal_rebuild and create the situation where the module file is loaded without a compiled container.

The test was failing as expected locally, but not on CI. Looking at the browser_output files from the test job, core/rebuild.php was returning an empty response, whereas locally the exception message was output to browser. My guess is that on CI, core/rebuild.php on the SUT is not set up to display errors, and hence the empty response.

However, when successful, core/rebuild.php redirects to the homepage. So adding an assert in the test to make sure we end up at the home page demonstrates the failure.

Nightwatch passed, everything lgtm now.

Rebased for the merge conflict.
Subform state should be accounted for now, and some logic issues fixed, with additional test coverage.

There is also 🐛 Regression: Drupal.displace() not working on new Navigation module in 11.2 Active , which should have been fixed, but might be affecting something here.

Updated deprecation version to 11.3 and refactored the test a little.

Nightwatch test failed, needs rerunning most likely.

If we write this file to /web/core, then an initial version of DrupalInstalled.php can be in the git repo for core, in that location, containing default values for the constants.

For my core development workflow, this would mean there'd be a modified DrupalInstalled.php file in my repo a lot of the time, which is an annoyance (probably minor) when rebasing etc.

We could alternatively protect against DrupalInstalled somehow being missing with a class_exists check:

class_exists(DrupalInstalled::class) ? DrupalInstalled::VERSIONS_HASH : \Drupal::VERSION

I made the proposed changes to Trait and Interface rules per #5 after all.

From the CR, this is not enforced for Interface. I tested this locally and can have a TestInterface class which isn't an interface. Do we need to add this?

I think it would make sense for both interfaces and traits, so if it's not too much scope creep, I'm for adding it to interfaces as well. But it is more important for traits because we do trait detection in 📌 Add a fallback classloader that can handle missing traits Active based on whether the what's being loaded ends in "Trait".

Could someone with this branch checked out confirm that an IDE can find the file containing the DrupalInstalled class from a place in the code where it's referenced?

PHPStorm finds the class, no problem.

The ValidatableConfig job in the build is failing, but it seems like that's because the job doesn't run composer install again after the switch back to the latest MR commit, and not an issue specific to this MR.

RTBC +1.

It's easy to reproduce with the combined MR, you just have to clear caches and load the umami front page to see broken rendering.

Tested this out locally really quick and I can see that drupalSettings.bigPipePlacheolderIds has 3 unreplaced entries.

{
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_views_block__recipe_collections_block&args%5B1%5D=full&args%5B2%5D&token=LSXAUcXQcqsNxZk01EJ-DceQbM_P3ovAc_wniqiM2X0": true,
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_views_block__promoted_items_block_1&args%5B1%5D=full&args%5B2%5D&token=XKowAdwoi2xYmp7GHvIqQteOfgS9deS_4OJckMkYGHQ": true,
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_banner_home&args%5B1%5D=full&args%5B2%5D&token=j1t-pelbFkxnp6XgWmQ0AG2woBVFHKrU7calg2QLe8M": true
}

One more thought: with named parameters, does this start to make constraint plugin classes or constructors API?