@godotislate

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?

A couple comments on the MR that look like bugs, not sure if related to the Umami test failure.

The simplification is straightforward. Re-ran test-only job and it fails as expected. LGTM.

I took a peek at the MR on 📌 Entity lazy multiple front loading Active to double check if it's related to anything here, and I agree it seems unlikely. I also added a couple comments on that MR about possible bugs.

Changes look good. The Build test failed, but probably just needs re-running. Also would be good to see the test-only job run and fail, then I think it's good for RTBC.

Didn't you already do the test only MR? https://git.drupalcode.org/project/drupal/-/merge_requests/11175

Yeah good for RTBC from me.

@alexpott FWIW, I think the proposed changes in #34 are fine. I also think that it'd be good to get this in as early as possible for 11.3, and I don't think that the file location needs to be a blocker. We can create a follow-up as needed if there are problems with the file location, as long as it's resolved by 11.3.0-alpha1?

Moving to Needs Review to get feedback on PoC MR. Also, there's functional test coverage for the specific issue, but will probably need additional tests for the new Interface/API introduced if we go with the approach.

Updated the IS proposed resolution to match the work done so far.

Because the execution is now inside a fiber, this suspend happens, then when it's resumed the cache is requested again.

Nice find.

To get a useful diff, I had to remove the hash salt and private key from the user permissions hash - I think we might also want to open a spin-off for that because not only are the permissions hashes never really exposed anywhere, but also there's no useful information in a hash as such anyway - this would allow us to compare cache operations much easier when they're part of cache contexts.

That comes from _ckeditor5_get_langcode_mapping(). Will open a spin-off to move that to a service and keep the mappings in a property etc.

Adding "Needs followup" tag as a reminder for these two.

I think only things left outstanding are the test failures in core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php and core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5MarkupTest.php. (The SettingsTrayBlockFormTest seems like a recurring intermittent failure.)

Interesting test failure with somehow the attribute order changing:

Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5MarkupTest::testAttributeEncoding
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'...e-images/image-test.png" width="40" height="20" alt="</em> Kittens & llamas are cute">'
+'...e-images/image-test.png" alt="</em> Kittens & llamas are cute" width="40" height="20">'

core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5MarkupTest.php:110

MR 12426 is ready.

Looked at the CR and made edits, including the line mentioned in #26, but it'd be good to get other eyes on it now, too.
MR looks good to me as well.

A couple things outstanding from #27:

Also I'd like to bring more docs over from the Drupal root issue, which IIRC had more docs on the code that writes the file.

Is there anything left to be done here?

What's the reason for putting it here rather than where core's files are put? Or in the webroot?

Only guessing at the reasoning, but for one, it'd involve a new .gitignore entry then, right?

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

A couple comments on the MR for minor docblock issues, and some nits that are fine to go without. I also tested this locally and the apcu prefix and cache container keys change as expected with changes to composer packages or running composer install after a git commit.

This is great for core because it means that any updates that introduce new services just work. But it is painful for contrib because it has to always add empty updates to indicate that a container rebuild is necessary.

Another is because we take the root project reference into account if your root composer is checked in to git and this repo contains your custom code then any change to custom code will cause a container rebuild (if you do a composer install afterwards).

Do we need a CR for the above?

Also, is it possible there are projects out there that aren't built with core-recommended or scaffold? If so, would there be an error because the DrupalInstalled class doesn't exist?

Addressed MR comment. I also removed custom assert messages per 🌱 [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages Active in the tests.

Also updated the CR and the issue title, because I think the word "placeholder" is confusing. Not sure exactly what Twig's terminology is for {{ ... }} syntax, but from the docs, there's this:

There are two kinds of delimiters: {% ... %} and {{ ... }}. The first one is used to execute statements such as for-loops, the latter outputs the result of an expression.

I've updated language in this issue title to replace "placeholder" with "expression" or "rendered expression" for lack of better terminology.

One other consideration, other than possible scale, for content updates, such as for block settings in layout builder overrides.
If content moderation is enabled, and there is a published revision and an unpublished forward revision, then both of those likely need to be updated, with care either to save the revisions in place or some other method to make sure the revision order is maintained.

Pushed up MR 12404 with a POC of a different approach, described as follows:

• Views has the interface DependentWithRemovalPluginInterface, with an onDependencyRemoval() method. When dependencies of a View entity are removed, View::onDependencyRemoval() checks each of the view's displays to see whether any views handler plugins implement the interface and method. If onDependencyRemoval() in the implementing plugin returns TRUE, then the handler plugin configuration is removed from the view and prevents the dependency removal from deleting the view config entity
• The idea here is to create a similar interface and method in the Core namespace, and that method should be invoked any implementing plugin in a config entity's plugin collection, when a dependency of the config entity is removed
• The core version of DependentWithRemovalPluginInterface::onDependencyRemoval() is slightly different from the Views version. Instead returning a boolean of whether the plugin should be removed or not, it returns an it, which is 1 of 3 values: -1 if the plugin settings have changed, 0 if the plugin settings have not changed, or 1 if the plugin should be removed from the entity's plugin collection. (This could possibly be an enum?)
• In ConfigEntityBase::onDependencyRemoval(), if the entity is an instance of EntityWithPluginCollectionInterface, then any plugin in any plugin collection that implements DependentWithRemovalPluginInterface will have its onDependencyRemoval() invoked, and the return value is reacted on appropriately
• To address this issue specifically, the MediaEmbed filter is updated to implement DependentWithRemovalPluginInterface, and in onDependencyRemoval(), if any media view mode is being deleted and is present in the plugin's "allowed_view_modes" or "default_view_mode" settings, those settings are updated to remove that view mode and set to use the "default" view mode as needed. The method returns that the plugin settings have been changed, so ConfigEntityBase::onDependencyRemoval() returns TRUE. The dependencies for the filter format are then recalculated and should prevent it from being deleted when the media view mode is deleted

The new MR also includes the test from MR 3115
(The Build tests has failed consecutively a few times, but I can't identify any related reason in this MR, so just going to let that sit for a bit.)
Moving to Needs Work for this POC and soliciting feedback for this change to config entity subsystem.

ensure that any media already embedded with a removed view mode falls back to the default view mode

Is this still something we want to do? Right now MediaEmbed renders an error "The referenced media source is missing and needs to be re-embedded" for media embedded with invalid view modes, so it would be a change in behavior to make it show in the default view mode as fallback instead.

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

Updated MR per feedback and adjusted tests.

The issue with this is how often do these blocks get resaved?

Here, it doesn't much matter because NULL/undefined is the same as FALSE for the new property, and FALSE is always a valid value.

It'd be an issue for any new block property that doesn't work like that. What I suggested in #15 could help address that, but if there are constraints where the default value is not valid with existing configuration, that would be another problem.

Yes, the issue with LB overrides mentioned in 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active , among others, is why I wanted to avoid doing an upgrade path. I think it's okay to have the config key introduced over time on block saves?

For block entities specifically, I wonder if we can change get()<code> so that calling <code>get('settings') will merge in configuration from the plugin collection. That way additions in defaultConfiguration() from the plugin will be picked up in the loaded block entity. That doesn't really change anything here, but it could maybe apply to other situations?

MR updated per #12.

Made the text changes and pushed. Thinking about this now, though:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

The class added in the core theme templates is menu-item--active-trail. There's also an is-active class added to a menu link if it is the current page, but that is added through JS. Still I wonder if "active" is not informative enough? Also, I'm not sure what the antecedent to the final "it" in the description text is.

My attempt/suggestion for an edit:

When this is checked, the menu will have the same markup on every page. Otherwise, the current page's position in the menu structure will be calculated and an "active-trail" class added to menu links in the current page's menu hierarchy.

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided. We'd probably also have to disable those fields when "track active trail" is unchecked, because #states can't change field values. Then in the form submit handler, we'd have to interpret those NULL values as level = 1 and expand_all_items as TRUE.

Yeah, I went with the negative to make the upgrade path easier (NULL being the same as FALSE), despite that it makes the wording maybe a bit awkward.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

Added more #states handling and the config schema validation constraint.
Also added functionaljavascript test for the form.

MR 12375 is up.
I added tests for the active trail functionality, but I didn't do one for the #states handling in the UI. Is that needed?

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1? Effectively it creates an empty menu if the start level is greater than 1, the menu is set to expand all items, and ignoring the active trail is enabled.

    // For menu blocks with start level greater than 1, only show menu items
    // from the current active trail. Adjust the root according to the current
    // position in the menu in order to determine if we can show the subtree.