🇺🇸United States @phenaproxima

phenaproxima → created an issue.

This breaks recipes that use Pathauto, which is many recipes in Starshot. Therefore, tagging this as part of those initiatives.

The fix looks correct to me, so RTBC on the assumption that tests will pass.

Duplicate of 📌 Automated Drupal 11 compatibility fixes for automatic_updates Needs review , which I'll fix in that issue. Take a hike, bot.

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

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

phenaproxima → created an issue.

phenaproxima → created an issue.

phenaproxima → created an issue.

Yeah, but we can't just rename the interface method, as that would break backwards compatibility.

The thing we need to do here is add a new interface method and deprecate the old one, assuming that's allowed under the backwards compatibility policy.

phenaproxima → created an issue.

📌 Add validation constraints to language.content_settings.*.* Needs work landed!

Looks good, I would suggest a few changes though.

If that passes, I don't see a problem here.

✨ Create flexible config actions to place a block in the admin or default themes Needs review allows position: first and position: last, but not ordinal positions. IMHO that would be nice to have.

This got committed in ✨ Create flexible config actions to place a block in the admin or default themes Needs review , so I think we can close this issue!

Fixed @alexpott's feedback. Since it was literally just renaming a parameter, I don't think we need to shuffle through the whole needs review process for this. Restoring RTBC.

📌 Fix Block config entity type config schema violations: weight, property Postponed is in!

Updating the issue summary with an partial list of every action added by this merge request, with examples for documentation.

Okay, I agree with you @tim.plunkett; did the deprecation dance around page_id and added a change record.

Assigning to myself to do some cleanup here.

Ship it.

Looks good to me.

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

I propose a slightly different variant: let's just allow the region to be a string, instead of an array. If it's a string, it's passed through to the EntityCreate plugin unaltered.

Glad we got this isolated, I think with that in mind it shouldn't be too hard to write a test case.

I have very few remaining complaints here! And frankly, they're pretty minor.

I think I've addressed all of @alexpott's feedback.

Alright, all done here AFAIK. I did the deprecation dance around non-integer block weights.

Per #3379725-32: Make Block config entities fully validatable → , this has to be handled over in that issue. Closing this out.

Can we do something more like GenericTest where we add a test for every recipe, but where that test is a minimal subclass of a recipe test base class?

We were hoping to eventually do something like this, but it wasn't a priority. But since core's test performance is being impacted, I guess this just became more important.

I would propose the following conventions and rules for recipe tests:

• They can be kernel, functional, or functional JavaScript tests. (I can't think of any way that recipes would benefit from unit tests.)
• They don't need a @group annotation.
• Core recipes are in the Drupal\(FunctionalTests|KernelTests|FunctionalJavaScripts)\Core\Recipe\recipe_name namespace.
• Non-core recipes are in the Drupal\Tests\Recipe\recipe_name\(Functional|Kernel|FunctionalJavascript) namespace (this is for another issue to decide; I'm not adding support for non-core recipes in this issue right now).

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

I removed the unnecessary test coverage, but as far as I can tell, this is otherwise good to go. I'm going to go ahead and boldly restore RTBC here, and if there's anything further to fix, let's get it over the line. This issue is even more imperative since ✨ Create flexible config actions to place a block in the admin or default themes Needs review is close to ready.

We might not end up needing this issue; region support is implemented in ✨ Create flexible config actions to place a block in the admin or default themes Needs review , but we may want to split that out. We'll see.

I see the follow-ups are already done, so this is ready IMHO.

From #60:

Personally I would say no, since the appropriate version of the action should be able to determine without extra effort on the part of the recipe author.

After sleeping on it, I think I agree with you here. It costs us very little to provide two actions with very clear names; I think that'll be good for recipe authors. Having to rely on a config input, while technically equivalent, is more complicated.

I added some keyword-based permission handling; it turned out not to be too tricky.

The main differences are that !8806 is a lot less "busy", and doesn't try to validate anything. We can rely on config validation for that, which is being added in 📌 Fix Block config entity type config schema violations: weight, property Postponed . So that's all gone.

I did some refactoring to make it a lot less verbose, and hopefully easier to follow. I also think it doesn't benefit us to try and handle every possible key of a block entity; we can just pass those to the EntityCreate action.

Things for follow-ups:

• Make this capable of updating an existing block. (This might necessitate moving the "change region", and "change position" parts to a new config action, maybe called moveBlock.)
• Give it the ability to handle absolute integer positioning, rather than merely "first" and "last".

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

We're not going to add any kind of filtering or sorting by most installed; this is strictly about defining and implementing the PHP API, and adjusting the JavaScript to match.

I wonder if we should consider blocking this on #3303126: Allow recipes to reuse existing config values → , since that would mean we could do stuff like this:

inputs:
  default_theme:
    from: config
    config: 'system.theme:default'
config:
  actions:
    block.block.foo:
      createIfNotExists:
        theme: %default_theme

phenaproxima → created an issue.

I guess I'll just restore RTBC, then!

To confirm, I was able to reproduce the problem in #17 with this command sequence:

composer install
php core/scripts/drupal quick-start standard
php core/scripts/drupal recipe -v core/recipes/standard

But that is not an idempotency issue, that's because the contact form recipe hard-codes an email address that's different from the one the Standard profile puts into the feedback form. This will be fixed as part of #3303126: Allow recipes to reuse existing config values → , so it's not in scope here.

I actually think sorting is fine as-is. It's inherently less complicated than filtering; how many different ways do we need to render a sort control? What could it contain except for "this the field to sort on, and this is its human-readable label"? That's already covered...

Looking pretty good; a few more suggestions. I think we also need test coverage of the changes to ExtensionExistsConstraintValidator.

I can't reproduce @Prashant.c's result in #13. My steps, with this MR checked out:

drush si -y # I confirmed that this installs Standard
php core/scripts/drupal recipe core/recipes/standard -v

Worked fine. What am I missing?

I spoke with @mandclu on Zoom and I think we arrived at an understanding on how to carry this issue forward.

In essence, I agree with the usefulness of a flexible block placement solution, but this MR is trying to do too much. We need to reduce its scope, and add more functionality in follow-up issues.

So why don't we start here: in this issue, let's define two new config actions: placeBlockInAdminTheme and placeBlockInDefaultTheme. If we want, they can be based on the same class, split by a deriver. This plugin should be a wrapper around the EntityCreate plugin class, and create a block entity. But the block's theme key should be automatically determined by the plugin. Beyond that, it should accept an array of entity values, just as EntityCreate would.

Then, before we commit it, let's create follow-ups for the following additional things:

• Flexible choice of region -- the block can go in a region specific to particular themes, or use a fallback region
• Flexible choice of position -- rather than specify a weight, the block can either go first in the region, last in the region, or at a specific absolute position.

Having to use an array keyed by the theme name makes the recipe unable to place a block in an unknown theme, which is one of the goals I want to achieve with this action.

I'm not sure we really want this, and I feel quite strongly that it's not appropriate.

"Place this block in a region named such-and-such, without knowing if it will actually make visual sense, or be desirable from a UX perspective" doesn't feel like a good idea to me. Truth is, recipe authors have no way of knowing what region names mean, and that's just a fact. I'm not sure it's appropriate for us to dance around it.

Also, iterating through an array and using the first match just feels complicated to me, from a recipe authoring perspective. I'd rather

I propose this as a compromise: how about an optional key, called default_region, which is the region it will use if region_map doesn't list the theme being targeted? If a default region isn't defined, then you get a ConfigActionException.

specify a theme, or use the 'default' or 'admin' keywords, the proper machine name will be determined, and other elements (such as the config name) will be updated appropriately

I think this should be represented as two different config actions, rather than keywords. That would make it impossible for there to be collisions with themes named "default" or "admin" (however unlikely that may be), and it means that it's impossible to screw up, because if you get the name of the action wrong, you'll get an exception.

the ability to include a "regions" array. If this is included instead of a single "region" string, the action will iterate through the array and use the first region that can be found in the determined theme

I don't think this is the best approach, because it is not predictable. Which region will it be? That depends on the theme. I think we can still create flexibility here, but have it be more predictable, by doing something like this:

region_map:
  olivero: some_region
  gin: some_region
  claro: some_region
  ...

If the current default or admin theme isn't in that list, you get a ConfigActionException. I like this because it lets the recipe be broadly compatible with any number of themes, but also guarantees that the recipe will always behave the same way in various contexts. We want recipes to be very, very predictable. IMHO this design would strike the right balance between flexibility and predictability.

In addition to accepting a numerical weight, a recipe author can use the "first" or "last" keywords, and the action will determine the necessary number and use that

I agree with this, although I think we should use numerical positions, not weights. Weights are an internal thing that might not be known ahead of time. Numerical positions are, again, predictable: if I say put this block at position 2, it will be at position 2, regardless of what the actual weight is.

There are also some attempts to provide sensible defaults for omitted keys, but happy to remove those if that's deemed a better approach.

For now, to keep things simple, I would suggest removing the extra key support for now. There would be nothing preventing a developer from creating the block entity with whatever keys they want, then using one of the placeBlock variants to adjust it.

phenaproxima → created an issue.

As an example, what if we implemented the action like this?

block.block.some_block_entity_id_that_must_already_exist_and_could_be_created_using_createIfNotExists:
  placeBlockInDefaultTheme:
    region: name_of_region_that_must_exist_in_the_default_theme
    at: end # can also be 'start', or an absolute integer position
    replace: false # if true, and 'at' is an absolute position, then the block already at that position in the region is replaced

There could just as easily be a placeBlockInAdminTheme action that behaves exactly the same way.

That rationale makes sense to me, and I agree about the somewhat limited nature of createIfNotExists. But, by that very same token, I think it also means we need to radically rethink the approach here, and make block placement more flexible in an incremental way, over multiple issues.

As an example, I see that we want to be able to place a block in the default theme or the admin theme, regardless of which themes those actually are. I think that small scope of work should be tackled on its own (and, in fact, it could already be done with the mechanism I've implemented in #3303126: Getting user input before applying a recipe → , so we might want to pursue getting that issue done).

If we need more flexibility from there, then we should do that in further, separate, tightly scoped issues. Trying to do too much at once -- adding too much flexibility at once -- is a greased slide to ill-defined APIs and unpredictable, buggy behavior. Which are things we are strongly trying to avoid in the recipe system, and in the config actions API.

We can talk about it later, but I'd love to get a sense of what kind of flexibility you'd like to see when placing blocks. That is, specific aspects of block placement that should be more flexible, and in what way.

The thing I'm unclear on is: what exactly are we trying to accomplish with this, as opposed to just using createIfNotExists?

I don't think this is ready to go, sadly. It's doing a lot more work than it should be, which makes it brittle. I'd like to take a stab at a more simplified version of this that basically is a nicer wrapper around createIfNotExists.

Never mind! It was a bit hidden, but it turns out that sessionStorage and localStorage can only store strings. From https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_t...

Storage only supports storing and retrieving strings. If you want to save other data types, you have to convert them to strings. For plain objects and arrays, you can use JSON.stringify().

Nice and neat.

phenaproxima → created an issue.

Coming along, but I think we can tighten it up a bit.