Account created on 17 August 2016, almost 9 years ago
#

Merge Requests

More

Recent comments

@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.

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.

Should this be moved to NW

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

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.

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.

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.

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

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.

@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

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?

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.

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?

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.

OK, confirmed that the changes for the build in[#3527555] for Project browser are passing (phpstan issues there are unrelated), and the changes there are the same as the changes in the MR 10809 here, other than the use of globstar patterns core/.phpunit-next.xml here. But that file isn't used in the PB build per https://git.drupalcode.org/project/drupal/-/merge_requests/10809#note_51..., so everything LGTM.

The suggestion in #18 is that Database-specific exception handling shouldn't be done outside the SQL content entity storage class, and per #43, the exception handling was moved to the content uninstall validator.

At the very least, I don't think all the exceptions should be swallowed without at least logging an error or warning and perhaps also showing a message. Theoretically at least Url::fromRoute() and EntityTypeManager::getStorage() could throw exceptions as well.

An alternative could be something implementing hasData() in SqlContentEntityStorage to be something like this:

  /**
   * {@inheritdoc}
   */
  public function hasData() {
    try {
      return parent::hasData();
    }
    catch (\Throwable t) {
      throw new EntityStorageException($t->getMessage(), $t->getCode(), $t);
    }
  }

Then catch EntityStorageException in the uninstall validator.

I made some code changes to fix other test failures, but I'll try to another look later today if I have time before pushing those up, since not all the test issues are resolved.

I've pushed these up in case anyone else wants to pick up. Haven't found solution to issues mentioned in #268.

CKEditor also now adds the "table" class to all <table> elements, so themes using CSS rules that style tables differently based on the presence of that class may need those rules changed.

Add above to RN snippet in IS.

@acbramley, took a peek at Drastically improve Drupal's default linking experience in text fields Needs work and, unfortunately it looks like the pain there would have been pain here had it gone in first, so lose-lose either way.

I also removed previewButton.setTemplate as I'm not sure if it is still required. Instead, I bind the preview button href and label property to the entity metadata.

There are failed tests, which I think are related to the UI changes, but I don't have the bandwidth to look at them at the moment.

I think the removed setTemplate() code has broken some functionality, especially with media and image links. The link preview for those does not show the entity label anymore.

This is a regression because it fails a test for that.

Among some other issues to resolve, the CSS for the autocomplete can bust the new link UI by pushing the "Insert" button outside the balloon:

I made some code changes to fix other test failures, but I'll try to another look later today if I have time before pushing those up, since not all the test issues are resolved.

To #45, agreed. I recall working on projects where the FE theme was styling tables based on presence of the "table" class, so I thought CKEditor adding that class to the table element to solve an editor display issue wrt centering was unfortunate.

MR for 11.x is 2000+ commits behind and unmergable

MR probably hasn't been touched in a year or so and has needed work since before that. It's a difficult challenge and open for anyone to implement a good solution.

Test failure, might just need rerunning?

The stylelint release notes are somewhat opaque, but it looks like there were quite a few rules added between 16.11.0 and 16.19.1: https://github.com/stylelint/stylelint/releases. Do they all need documentation in the CR?

OK, MR for 10.5.x with changes per #27 for no deprecation warnings: https://git.drupalcode.org/project/drupal/-/merge_requests/12331

If I'd thought about it more, would have waited to see if any package versions would be changed in 11.x MR, but should be easy enough to copy over if so. Held off on 10.6.x for that reason, though, assuming 10.6.x needs an MR as well.

@xjm added MR comment about the ckeditor5.essentials version.

New MR 12331 is for 10.5.x and WIP.

I noticed one outdated license reference in the MR

Looks like ckeditor5.essentials in core.libraries.yml is at version "35.1.0" and linked to the the corresponding license as well. Didn't see any history about that with a quick look at the git blame.

Updated the title to match the new direction of this issue to remove memory management code in migrate. Also removed the "Needs follow up" tag, since it referred to messaging in migrate that won't be relevant anymore.

I agree with pushing off a better BC layer to a follow up. It's non-trival effort, and use cases seem to be limited to adding buttons to the Media dialog. Also, one thing about specifying icons is that they can either be the SVG markup or name of an existing icon, so the BC detection needs to account for that as well.

Added the version constraint changes to core/composer.json.

Removed the changes to composer/ca-bundle, composer/spdx-licenses (any non-Symfony packages).

We have tried applying the MR 11571 as a patch, but the result is that we get this message instead of the language block content:

At the very least you'd need to have the commit for 📌 Review cache bin and cache tags of access policy caching Active , which introduces CacheOptionalInterface, for the MR diff here to work correctly. It won't be available in any stable release until 10.5.0, 11.2.0, or the next release of 11.1.x.

Split off updating Symfony to 7.3.0 from 📌 Update Composer dependencies for 11.2.0 Active , per #13:

Symfony 7.3.0 is also now out, and could be given its own issue as it's more critical than other updates.

Symfony 7.3.0 is also now out, and could be given its own issue as it's more critical than other updates.

📌 Update to Symfony 7.3.0 Active

Since the draft MR here is green, just cherry-picked that commit to the MR there. https://git.drupalcode.org/project/drupal/-/merge_requests/12317

Production build 0.71.5 2024