🇺🇸United States @michaellander

michaellander → created an issue.

michaellander → created an issue.

How embarrassing. This was a screw up with git on my part. A new alpha is released with the correct latest code.

If we wanted to cover that part in the ECA schema as well, we would have to have an aggregate of all the plugin schemas in the ECA schema. Or is there a way to refer to the plugin-specific schema by the plugin ID, including for all those unknown plugins that come from contrib modules?

You only need to define schema for the actions you are defining, and then you are correct, you can just have an argument for the plugin_id to handle all actions, irregardless of if they are in your purview.
Here are a few Action schema snippets from core:

//from web/core/config/schema/core.entity.schema.yml
action.configuration.action_send_email_action:
  type: mapping
  label: 'Send email configuration'
  mapping:
    recipient:
      type: string
      label: 'Recipient'
    subject:
      type: label
      label: 'Subject'
    message:
      type: text
      label: 'Message'

and

//from web/core/modules/user/config/schema/user.schema.yml
action.configuration.user_add_role_action:
  type: mapping
  label: 'Configuration for the add role action'
  mapping:
    rid:
      type: string
      label: 'The ID of the role to add'

Which would be covered by:

   type: action.configuration.[plugin_id]

michaellander → created an issue.

Looks like we might also want FunctionCall Attribute to extend the Drupal\Component\Plugin\Attribute\Plugin class, and then the FunctionProperty might more closely reflect the changes 📌 Convert ContextDefinition plugin discovery to attributes Closed: won't fix in regards to using constraints and what have you.

It might be worth taking inspiration from how the core Context API works for this. In particular, I think it makes sense:

1. Move the FunctionProperty definitions into the FunctionCall definition. This would give you visibility into the FunctionCall props prior to instantiating the plugin. An example context definition in another plugin:
   

   context_definitions: [
       'entity' => new ContextDefinition(
         data_type: 'entity',
         label: new TranslatableMarkup("Entity"),
         required: FALSE,
         description: new TranslatableMarkup("The contextual entity that can be used for token replacements.")
       ),
       'recipient' => new ContextDefinition(
         data_type: 'email',
         label: new TranslatableMarkup("Recipient"),
         description: new TranslatableMarkup("The email address to which the message should be sent.")
       ),
       'subject' => new ContextDefinition(
         data_type: 'string',
         label: new TranslatableMarkup("Subject"),
         description: new TranslatableMarkup("The subject of the message.")
       ),
       'message' => new ContextDefinition(
         data_type: 'string',
         label: new TranslatableMarkup("Message"),
         description: new TranslatableMarkup("The message that should be sent.")
       ),
     ]
   

2. Additionally we could tap into cores DataType API and 'Constraints', similarly to Contexts. This would make it easier to describe and reuse existing property types, and set more advanced rules like 'string' with max length 84. For example:
   

     constraints: [
       "ComplexData" => [
         "value" => [
           "Length" => ["max" => 84],
         ],
       ],
     ]
   

3. Consider interfacing/extending Drupal\Core\Executable\ExecutablePluginBase and/or Drupal\Core\Executable\ExecutableInterface, though ExecutablePluginBase does have an implied config layer that might not really align with properties

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

I removed the remaining tasks for now, it was previously set to 'Review Patch', but that definitely was not sufficient.

In the fork I created, the things I was still sorting through:

• execute / executeMultiple solution and infinite loops
• Node actions extending entity agnostic Drupal\Core\Field\FieldUpdateActionBase, need means to know name of entity context
• Backwards compatibility with Action plugins from contributed modules.

I appreciate the comments and I'll look at the Rules approach to it.

Looks like it still needs work around access calls.

Reviving this as I believe Action plugins may be a great pathway to unlock core and contrib en masse as AI tools. However, to do so, actions would need to be able to communicate required contexts.

Much has changed since the last patch, so I tried to work through and map changes accordingly.

I did change the approach for:
Drupal\Core\Action\ActionBase::executeMultiple with the hopes of better backward compatibility.

Original:

/**
   * {@inheritdoc}
   */
  public function executeMultiple(array $entities) {
    foreach ($entities as $entity) {
      $this->execute($entity);
    }
  }

From most recent patch:

  public function executeMultiple($context_name, array $context_values) {
    foreach ($context_values as $context_value) {
      $this->prepareExecutionContext([$context_name => $context_value]);
      $this->execute();
     }

New:

/**
   * {@inheritdoc}
   */
  public function executeMultiple(array $contexts) {
    $context_names = array_keys($this->getContextDefinitions());
    foreach ($contexts as $context_values) {
      if (!empty($context_names) && $context_values instanceof EntityInterface) {
        // Assume a single context of type entity exists.
        $context_values = [$context_names[0] => EntityContext::fromEntity($context_values)];
      }
      // Assume the plugin hasn't been upgraded.
      else {
        $this->execute($context_values);
        continue;
      }
      $this->prepareExecutionContext($context_values);
      $this->execute();
    }
  }

Based on this change, I was looking for feedback of how:
Drupal\user\Plugin\Action\CancelUser and Drupal\user_batch_action_test\Plugin\Action\BatchUserAction used execute and executeMultiple

I also wasn't sure if I needed to add constraints as well?

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

michaellander → created an issue.

Added as maintainer! Appreciate your interest!

michaellander → created an issue.

If you are on 10.1 and above, try the latest 2.0.x release. If you are on 10.3 and above, try the latest 2.1.x release or test the 3.0 alpha. Let me know if you are still seeing the issue!

Moved this into 3.x.. I'm not opposed to backporting it, but my concern is how significant of a change in approach it is from 2.x versions. The new 3.x alpha should be all the latest with this, and then some plans for a few additional improvements elsewhere.

@JeroenT I'm going to move the CI stuff into a separate issue and close this. Thanks!

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

We've been using the 'Overview' approach patch from ✨ Allow modules to hook into top of content/footer sections of new core navigation Active as a stop gap because users were getting so frustrated they couldn't access the 2nd level when a third level item was added. In our particular example it was a combination of the commerce → and scheduler → that was making it impossible to access the commerce dashboard.

In this particular use case I don't believe commerce should define a submenu link to 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.

I'll be honest, so long as we treat the dynamic between the 1st and 2nd level different from the 2nd and 3rd level, where the 1st level is linked at the top of 2nd level, but 2nd level is not accessible at all if 3rd level exists, I believe we'll always have this confusion. I think this also includes hiding/showing behavior as related to \Drupal\system\Controller\SystemController. Perhaps an additional property could be added for Menu links to define an 'Override' title when children exist, but again that seems like something that should be consistent between the 1st and 2nd levels as well.

It's for these reasons that I think the split button is most elegant, but I'd even take the 'Overview' solution over no solution.

I went ahead and created ✨ Allow modules to ALSO hook into top of footer section of new core navigation Active so that we could come back to it when timing is more appropriate.

michaellander → created an issue.

This feels like a duplicate of ✨ Add "All" or overview links to parent links Active , which feels like a duplicate of 🐛 Second level menu items can't be reached if they have children Active .

Added. Thanks for your contribution!

Looks like this may be related to or a duplicate of 📌 Parent menu items with URLs are not linked to, do we make these available? Closed: outdated

I think part of the challenge we are finding while using navigation every day, is a module might add an admin link to the navigation, without any children, with the expectation that a user goes there to configure it. Other modules might extend that modules functionality and add sub pages. So a page that may have originally stood on it's own inadvertently becomes the overview for other modules. I definitely get the idea around eliminating the generic overview link list pages, but in an open ecosystem, it's definitely likely that a sole page could later become a parent.

With progress over at ✨ Integrate with Workspaces Active . We may be able to start to adapt our approach to work more similarly, realizing things are still changing. I'll try to find time in the next few weeks.

Nice work! For the switchers, we could probably just use Drupal\Component\Utility\Html::getClass() on the environment label to generate a wrapper class name, and use that class to switch the variable values in a given context. Could always wrap the name generation in a utility function/method?

:root {
  --environment-indicator-bg-color: blue;
}
.environment-indicator--production {
  --environment-indicator-bg-color: red;
}
.environment-indicator--develop {
  --environment-indicator-bg-color: green;
}

Thoughts?

Thanks for creating the issue! For anyone interested, I put some of this in ✨ Support for core navigation experimental module Needs work .

Specifically:
https://git.drupalcode.org/issue/environment_indicator-3457688/-/blob/3457688-support-for-core/environment_indicator.module#L144

However, I stopped short of adding it for non-active environments. Was looking for more feedback!

This should be working better now.

I had this consistently working and now with a fresh install it's not... needs more work.

This should give us a path forward for modules like ImageAPI Optimize AVIF → to run alongside this module without needing glue modules like ImageAPI Optimize AVIF & WebP → .

If we can get some more eyes on ✨ Attempt to remove ImageStyleDownloadController override completely Needs review , I believe it'll make this update much easier overall. I think it allows us to keep this in 2.1, and should give us a path forward for modules like ImageAPI Optimize AVIF → to run alongside this module without needing glue modules like ImageAPI Optimize WebP and AVIF →

This version seems to fix issues around overriding the Drupal\image\Controller\ImageStyleDownloadController completely and is much more elegant. We will need to test to make sure everything is flushing correctly when the image is replaced.

Also going to test ✨ Attempt to remove ImageStyleDownloadController override completely Needs review , which would really simplify things. In which case maybe we just move to 3.x if it works.

michaellander → created an issue.

@danielwirz, am wanting to include 📌 Automated Drupal 11 compatibility fixes for imageapi_optimize_webp Needs review in a 2.1 release so we can confidently support Drupal 11. If you can help test the patch in that issue on 10.3 and 11, that'd be great.

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

I've added the necessary core_version_requirement and tested in Drupal 11, and all looks good.

michaellander → changed the visibility of the branch project-update-bot-only to hidden.

michaellander → changed the visibility of the branch 3431022-automated-drupal-11 to hidden.

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

I've created a 2.1.x branch for 10.3 and above. I need to test the compatibility of the D11 fixes with 10.3, found here:
https://www.drupal.org/project/imageapi_optimize_webp/issues/3448709 📌 Automated Drupal 11 compatibility fixes for imageapi_optimize_webp Needs review

And then I'll create a 2.1.0 release. We should probably upgrade 2.0.x to not allow anything above 10.2.

Hiding patch and moving to branch.

michaellander → changed the visibility of the branch 3453941-declaration-of-drupalimageapioptimizewebpcontrollerimagestyledownloadcontrollerdeliver to hidden.

Looks good, thank you!

@ckrina - These look awesome! Appreciate the feedback. Definitely would be great to be able to use the same templates as core.

@bronzehedwick - I think it's because some of my css depends on the toolbar module being enabled(unintentionally).

I'm also using CSS variables for part of it, but in some areas I am using inline styles, which is the existing approach. I think it would be nice to align the approach to be the same (All css variables, or all inline styles). I've just been waiting for feedback and maybe things to catch up a bit!

The module has already set precedent for handling other toolbars without submodules. When Navigation is the defacto standard, do you intend to move the ToolbarHandler behavior into a submodule as well? I think it's a rather confusing experience that if you are using the older toolbar, you just install Environment Indicator core, but if using the new Drupal core Navigation, then you need the submodule. We could always mimc the ToolbarHandler approach in making it responsible for checking if it's applicable(and just move the lazy rendered in there). The submodule just feels unnecessary.

With Navigation being in core(since 10.3), I don't think it makes sense to require a submodule. And any hooks required simply just won't fire if navigation doesn't exist.

@daddison, good question. I believe I wrote it with layout_builder_restrictions_by_region enabled, but I don't believe it is required. It should just be respecting whatever the scope of permissions(section vs region) you have configured. The test scenario will be that the region won't even highlight for you to drop a block in unless it's allowed, instead of waiting for you to drop the block and then get the pop-up warning. If you are testing locally, and have cache turned off, along with a lot of dev tools enabled, it may be kind of slow to validate. Though we've been using it in production on a handful of sites and thus far the performance lag has been negligible.

This is really cool! I'd be curious if there's areas it would make sense to start and define UI/design conventions around its use. I think the fade in/out makes sense in most cases because it's subtle, but maybe areas like Same Page Preview being able to slide the edit form over to fade in the preview, etc etc..

I'm still a bit conflicted, in that I think we should just allow modules to extend on the layout builder usage, and also allow users to add blocks to the navigation footer. However, I realize there's some hesitancy here, so we still should consider giving some means for modules to extend these areas.

michaellander → created an issue.

Looking into it more, we'd probably want to remove the dependency for gin_toolbar and core's toolbar and instead would want to conditionally work based on what modules exist on the given site. Definitely needs more work and maybe some guidance from maintainers.

It looks like the code is not correctly handling when the 'toolbar' module is turned off, so this needs more work.

Updated the css to be a bit closer to concepts:

We started using layout builder in mega menu's awhile ago with great success, and seeing layout builder implemented in the new navigation builder successfully is awesome. I'd love to see it used here. We could have blocks for 'Entity Operations', 'Form Actions', etc. With the left toolbar acting more global, and the top bar holding more contextual info, we could take advantage of the existing Context system for some of the block visibility handling. This approach makes it far easier for other modules to extend and override the top bar without having to override templates everywhere.

I'd be happy to build that in similar fashion to the left hand toolbar if it would be helpful.

Confirming @Balu Ertl's fix, both modules just need the info.yaml update. I went through the environment switcher UI to create new environment entities and everything was fine. Active environment settings in settings.php also was fine. No issues in the logs.

I pushed up another version:

I did an initial attempt at the CSS, but my CSS nor UI design is my strength. I did use an approach with CSS variables, and I did not create a new Handler class. If we do want to go the Handler route, maybe we take advantage of service tagging in the early returns to avoid hardcoding every potential Handler check.

I also have left out the 'configure' link, but not opposed to adding that back either. I could probably just use feedback and/or design inspiration before going further.

I've made decent progress on this(see blue button on bottom left):

@ckrina, in the version you have, there are no child links to actually do the environment switching. Did you have another idea of where that belongs or is my approach on the right track(with some cleanup)?

It's using a lazy builder similar to many other blocks in navigation so that we can respect which environment links to show per the users permissions. I should be able to have a fairly complete version pushed back into the fork by tomorrow.

Is it worth including some examples to pressure test on the extreme end of low-code customization? For example a heading where you can adjust font-style, font-weight, heading level, margins(top, bottom, left, right), padding(top, bottom, left, right), display, position(top, bottom, left, right), text properties(color, align, decoration, etc), height, width, line-height, etc etc...

This would be both useful from performance/storage perspective, but also in some UI decisions.

I like that as an interim solve, and would allow us to pivot in the future if core makes the blocks that are allowed override-able. I'd be happy to push this forward while you're gone.