🇬🇧United Kingdom @alexpott

I'd rather decide how we're going to implement the composer plugin first before we add this. There's lots of efforts to try to make recipes work like modules and I feel this is a mistake. The recipe runner looks were you tell it to. As far as I know it does not look in /recipes. This change needs more justification as to why it is correct.

Committed and pushed 539c50a257 to 11.x and 55c348aaaf to 11.0.x. Thanks!

I can confirm that the build does not result in any changed files.

Committed 2ad2c6b and pushed to 11.x. Thanks!
Committed a71d5ea and pushed to 11.0.x. Thanks!

Committed 06881e8 and pushed to 10.3.x. Thanks!
Committed 24e0148 and pushed to 10.4.x. Thanks!

Well I think we should move all the active checks out of \Drupal\Core\Recipe\Recipe::parse() and into a validate method. This should make constructing recipe objects light enough for a general Recipe discovery mechanism and make things easier.

Ie… the calls to

validateExtensionIsAvailable
validateConfigActions

Should go into a validate() method. And the code in ConfigConfigurator::construct should also move into a validate function.

Uploading a patch for testing in the thunder project...

This project uses the PHPCS checks provided by core and runs them using gitlab ci - everything is currently green.

alexpott → created an issue.

alexpott → created an issue.

alexpott → created an issue.

alexpott → created an issue.

Content lock 2.4 has been released - this is a duplicate of the issue that fixed this.

Okay - we've got test coverage of the view and saving the view while trying to change the cache plugin. This is looking good. I manually tested the post update and it is was working.

Adding a test and changed a few things.

alexpott → created an issue.

alexpott → created an issue.

I think the implementation here is fine and I think testing this beyond that is has not broken anything is fine. And the manual testing provided by @seanB is good. Just a small nit on the MR to address. Also the fact that so many queries are necessary on the page is a bug.

FWIW the test now fails when run without the fix. https://git.drupalcode.org/project/drupal/-/jobs/1598345

Committed 98ae2ff and pushed to 11.0.x. Thanks!
Committed f560c83 and pushed to 11.x. Thanks!

I think the claim that

The problem is that...it outputs the error, then continues executing! This leads to buggy behavior.

Is actually incorrect. What happens now is that an exception is thrown when we call $recipe = Recipe::createFromDirectory($recipe_path);... there's no buggy behaviour at all. The exception comes from \Drupal\Core\Recipe\Recipe::parse() so very early on. And the exception message is accurate too.

alexpott → created an issue.

I've opened 📌 Move RecipeDiscovery into RecipeConfigurator Needs review to remove the existing RecipeDiscovery. I'm not sure we need this work yet in core but at least there will less confusion about the purpose of the class that was called RecipeDiscovery.

alexpott → created an issue.

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

Committed and pushed aeb50aea4c to 10.4.x and afd3e94a44 to 10.3.x. Thanks!

@thejimbirch thanks for working on this. I'm happy to be in MAINTAINERS.txt for default content and recipes.

alexpott → changed the visibility of the branch 3445525-event-dispatcher-bc to hidden.

alexpott → changed the visibility of the branch 3445525-issue-2909185-breaks to hidden.

This change is making public API changes - yes those variables are part of the public API as we can see in core's usage - for coding standard reasons. Given this is not really Drupal's code I'm not 100% convinced about the value of making this change - and it's too late for the 10.3.x ship. I think we should deprecate in 11.x and remove in 12.x if we want to make this change. I guess changing to typed properties is decent API ensuring bonues.

Committed and pushed de44e09794 to 11.x and 09341e6f28 to 11.0.x and 8030629daa to 10.4.x and 6620c86ab1 to 10.3.x. Thanks!

looks great - given these are bad method calls throwing implementations it's not hard to remove them.

Left a comment on the MR. I think we need to always mark for reindex. The reindexing is not language specific but doing this is not going to recreate any row.

pameeela → credited alexpott → .

pameeela → credited alexpott → .

Committed and pushed 01da0fd920 to 11.x and 5923aeaed8 to 11.0.x and fa05fb7a93 to 10.4.x and 2d32ea4961 to 10.3.x. Thanks!

Committed and pushed 655cb760fc to 11.x and 0bc782b1f8 to 11.0.x and 24b4a8ea8e to 10.4.x and 04fb2d1d1e to 10.3.x. Thanks!

diff --git a/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
index 132248d8fc..70047c8983 100644
--- a/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
@@ -22,7 +22,6 @@
 use Drupal\media\Plugin\media\Source\OEmbedInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
-// cspell:ignore allowtransparency
 /**
  * Plugin implementation of the 'oembed' formatter.
  *

Made the change above on commit because allowtransparency is no longer in the file.

Committed and pushed 565a9f1401 to 11.x and 4874e83fb3 to 11.0.x and 9181970ca4 to 10.4.x and 0f4d8306b1 to 10.3.x. Thanks!

Committed and pushed 356ba1fc65 to 11.x and 5dd92df9f7 to 11.0.x. Thanks!

Committed and pushed a52cdfad63 to 11.x and 007556a098 to 11.0.x. Thanks!
Committed and pushed b10e96de15 to 10.4.x and be2b4ec313 to 10.3.x. Thanks!

@bbrala as long as it applies to 10.3.x we're fine - and it will. Given this only text changes I think the rtbc from #13 stands.

Can we change the 10.4.x deprecations to be for 10.3.x - I think it is the best option because I think doing 10.3.x deprecation and remove in 11 would put us in a better situation wrt to security. Having something that can do file uploads around for the whole 11.x cycle that is unused and untested feels like a v bad idea.

Sure we can open an issue to make this option not limited to just node but that does not feel like a specific follow-up of this issue.

Committed and pushed 6d3cdb7701 to 11.x and 10be645825 to 11.0.x and 99ed4332d1 to 10.4.x and e4a770899b to 10.3.x. Thanks!

I was wondering about upgrade paths because cache tags appear in views config - but these cache tags sure don't because they are way more dynamic.

Committed and pushed a61eee36d2 to 11.x and 7201816d30 to 11.0.x and b676d86b91 to 10.4.x and d3adef268d to 10.3.x. Thanks!

Nice catch @longeave given it was just a nit back to rtbc.

There are failed tests - can the MR be updated for the latest changes and have a green test run.

Hmmm… but how to have a test that uses annotations compatible with D10 (on PHPUnit 9) and D11 (on PHPUnit 11) - just not care about the deprecations? I guess if you are going for D10 and D11 compatibility then you should be ignoring deprecations. So we just need to document that.

Here are some steps to implement:

• You need to set the repo for this project to use a different gitlab ci yml file - you can do this in the gitlab settings
• The custom gitlabci.yml (maybe change the name) file: https://git.drupalcode.org/project/distributions_recipes/-/blob/11.x/.re...
• Create a phpunit.xml.dist from core's file in the root and list your initiative's test directories
• Create an empty phpstan-level9-baseline.neon file in the project root
• Do some other things that i have forgotten but I'm more than happy to help with

You get a lot of advantages doing it this way:

• Your own issue queue to work on things
• Ability to commit incremental change because you are not relying on a core committer to review or everything to be ready for core
• Complete control of what tests are running on your issues
• Code coverage reports on your code
• Ability to increase PHPStan level on your own
• Auto create a patch for people to apply to core to preview your work
• When it comes time to land the core MR it's not to hard because you add the MR fork as a remote to the same repository and you can continue your separate workflow to handle core MR comments and then cherry pick to the core MR branch...

@tim.plunkett thanks for the review - it's the expected failure because we're doing:

    $configurable_plugin = $this->prophesize(ConfigurableInterface::class);
    $nonconfigurable_plugin = $this->prophesize(PluginInspectionInterface::class);

So these are Prophecy classes that implement the respective interface.

If we block requests to anything other than the site under test we're going to break contrib and custom tests big time.

I think we should open a follow-up to add a post update to clean up the incorrect data. This issue stops the rot but the bogus data hangs about.

Seems tests are failing.

Missing build tests - but they might mess with the working directory even more.

Committed and pushed 7deaa481d9 to 11.x and b00ae16892 to 11.0.x and 2d27a17771 to 10.4.x and 91b7e66dd3 to 10.3.x. Thanks!

This seems simple enough to backport all the way to 10.3.x

alexpott → created an issue.

#15 doesn't work - because that test is a config sync. Oh well. Agree that testing this would be hard.

FWIW testing this would have been quite simple - I think we could have to install a module in \Drupal\FunctionalTests\Installer\InstallerExistingConfigNoProfileTest and it would have crashed... ie add something like:

  /**
   * {@inheritdoc}
   */
  public function testConfigSync() {
    parent::testConfigSync();
    // Ensure language_modules_installed() works without an install profile.
    $this->assertTrue(\Drupal::service('module_installer')->install(['syslog']));
  }

I'll open a follow-up when I have time.

alexpott → created an issue.

alexpott → created an issue.

I've filed this issue against 10.3.x because it occurs due to the 10.2 to 10.3 upgrade but it is also present in 11.x

alexpott → created an issue.

Committed and pushed 152b7f0694 to 11.x and a6b7ab7388 to 11.0.x. Thanks!

Committed 9c5eeca and pushed to 10.4.x. Thanks!
Committed 6871694 and pushed to 10.3.x. Thanks!

I don't think we should do the suggestion with method exists as this is called a lot - so doing the ignore in PHPStan and the version check in 10.x seems fine.

@smustgrave nope - the messages on the issue about reverts and commits from 11.0.x are the wrong way around.

Added a comment that shows we have coverage of the problem in the issue summary by reverting a change to the test code that was made in the earlier issue.

Also the test only job fails as expected - https://git.drupalcode.org/project/drupal/-/jobs/1534956

Fixing the title

Lol #15 was totes a silly mistake... causing loops.

So for some reason \Drupal\KernelTests\Core\Routing\ExceptionHandlingTest::testExceptionEscaping and \Drupal\KernelTests\Core\Routing\ExceptionHandlingTest::testBacktraceEscaping are breaking with this change.

It looks like the kernel tests go into a loop on gitlab ci - locally they just say "Test was run in child process and ended unexpectedly" :D

Added some questions/comments to the MR

Also #55 is not addressed in comments.

Also #50 is not really addressed either - i.e. the part about what the form system adds to all elements by default and will be missing if you add elements in after build.

alexpott → changed the visibility of the branch 9.3.x to hidden.

alexpott → changed the visibility of the branch 10.1.x to hidden.

alexpott → changed the visibility of the branch 11.x to hidden.

alexpott → changed the visibility of the branch 3027240-undefined_parents to hidden.