🇧🇪Belgium @DieterHolvoet

Seems to be a failing field_ui test, I'm going to do some more manual testing to make sure the clone addition doesn't break anything.

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

I pushed a new approach where the old route requirements are stored in a route option, and during the access check these requirements are also checked and OR'ed with the checks of this module. This abstract approach should fix most issues.

Started a MR based on 2996548-menu_admin_per_menu_compatibility-12.patch.

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

It seems like adding menu_name to the context array is the best realistic solution at the moment. I rebased the patch against 11.x and injected the current route match as dependency.

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

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

There are existing issues dealing with that, I referenced them in the related issue list.

None of the parent classes/interfaces have a return type yet, so leaving it out until they do seems safer to me.

DieterHolvoet → created an issue.

Looking at the source of the dotenv:dump command, it seems like project_dir should always be the Composer root of the project. Since we're already using DrupalFinder in the dotenv:init command, I decided to use this here as well to automatically determine the project_dir. Could you check if the changes in this MR fix the issue for you?

I also made it so dotenv.project_dir can be set in services.yml, but this shouldn't be needed for you anymore. Setting dotenv_path in composer.json also shouldn't be needed anymore.

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

Just tested it myself and it looks like that's not necessary, values are being casted automatically.

You're right, that fixes it for me. Thanks!

That's it! The following class in my custom code seems to be the culprit. I don't think I'm doing anything wrong there though?

<?php

namespace Drupal\my_module\Config;

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Config\ConfigFactoryOverrideInterface;
use Drupal\Core\Config\StorageInterface;
use Drupal\Core\Session\AccountProxyInterface;

class MenuBreadcrumbConfigOverrides implements ConfigFactoryOverrideInterface
{
    public function __construct(
        protected AccountProxyInterface $currentUser,
    ) {
    }

    public function loadOverrides($names): array
    {
        $overrides = [];

        if (!in_array('menu_breadcrumb.settings', $names, true)) {
            return $overrides;
        }

        if ($this->currentUser->hasPermission('access toolbar')) {
            if ($this->currentUser->hasPermission('access editor toolbar')) {
                $overrides['menu_breadcrumb.settings']['menu_breadcrumb_menus']['editor']['enabled'] = 1;
            } else {
                $overrides['menu_breadcrumb.settings']['menu_breadcrumb_menus']['admin']['enabled'] = 1;
            }
        }

        return $overrides;
    }

    public function getCacheSuffix(): string
    {
        return 'MenuBreadcrumbConfigOverrides';
    }

    public function getCacheableMetadata($name): CacheableMetadata
    {
        return (new CacheableMetadata())
            ->addCacheContexts(['user.permissions']);
    }

    public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION)
    {
        return null;
    }
}

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

DieterHolvoet → created an issue.

I added a simple check to make sure that the preview is only refreshed (opened) if it's already open.

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

Oh and might be worth mentioning, the user.permissions cache context is attached to the role entity, which at some point is added as cacheable dependency.

Yes, Xdebug is enabled.

DieterHolvoet → created an issue.

Here's another idea: why not add our own Shortcuts dashboard block instead of altering the core one, without using a lazy builder? I started a new MR.

DieterHolvoet → changed the visibility of the branch 2.1.x to hidden.

1. You changed the theme hook to dashboards_admin_shortcuts, which doesn't exist.
2. Wouldn't it make more sense to either fix the custom theming or to get rid of it altogether? The current solution just leaves a bunch of dead code that will never be executed.

I rebased the MR on 8.x-3.x.

DieterHolvoet → created an issue.

Or even easier, can’t you set the parameter in the services.yml file? If the service provider overrides that value, we could add a check to make sure to only set a project path if there isn’t any yet.

I'm obviously okay with the config schema change, but I'm going to skip the other unrelated changes. Especially the introduction of the PHP attribute, since that would require us to drop support for certain older Drupal versions and I don't feel like that's absolutely necessary at this point.

Do we need to add an update hook that converts existing preserve_placeholders values to booleans or is this something which is being done automatically?

At the moment this module doesn't have any config and I feel like this use case might be a bit too niche to justify introducing config. Why don't you create a custom module with a service provider where you set the dotenv.project_dir container parameter to a different value?

This would be fixed if we could somehow not let Tagify filter the results. So only Drupal does filtering, Tagify just shows the list of results that's returned by Drupal.

I created 2.2.0 & 3.0.0 releases.

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

The config schema for required_plugin_options will have to be provided by the respective modules. This is what it looks like in my WIP required_by_content_moderation_state module:

# config/schema/required_by_content_moderation_state.schema.yml
required_api.plugin_options.required_by_content_moderation_state:
  type: sequence
  label: 'Content moderation states'
  sequence:
    type: string
    label: 'Content moderation state'

DieterHolvoet → created an issue.

There is a visual difference: this approach makes it so the delayed required fields still show the required asterisk, but they don't trigger a form error. Personally, I like this even better.

I replaced the overridden EntityFormDisplay with an overridden FormErrorHandler, that seems to do the trick.

Seems like the first chance to get an entity object which is updated with the last form changes is in ContentEntityForm::validateForm(), after $this->buildEntity() is called. It seems like at that point, the required validation errors are already present in the form state though. This means that it's impossible to determine the required status based on the entity object, if we want to change the required status before the form is built/validated. The alternative is to do it after the form is built/validated and to remove any unnecessary required validation errors from the form state. I'll try out this approach in the MR.

Okay, this isn't it yet. Drupal\required_api\Entity\RequiredApiEntityFormDisplay::buildForm() isn't executed when submitting the form, so any new values aren't being considered in the required logic.

The reason for this refactor is my intention of releasing a required_by_content_moderation_state module, allowing fields to only be required when their associated entity reaches a certain Content Moderation state. Before, this would only be necessary by reading parameters of the global request, and even then it wouldn't work when programatically validating entities. After the changes in this MR, all these things work.

I pushed a pretty big refactor of the module:

• I replaced the original approach with an approach where I override the EntityFormDisplay entity class in order to have access to the field entity
• I added an extra approach where I override the TypedDataManager & RecursiveValidator classes in order to add support for validating entities in non-form contexts, e.g. when executing Drupal\Core\Entity\FieldableEntityInterface::validate().
• I removed the $account argument of the Drupal\required_api\Plugin\RequiredPluginInterface::isRequired() method, because it can be replaced with an injected current_user service
• I added the $entity argument to the Drupal\required_api\Plugin\RequiredPluginInterface::isRequired() method, in order to have access to e.g. other fields to determine whether or not a field should be required.

Let me know what you think!

Adding this argument will result in backwards-incompatible changes, but since a new 2.x branch was started we can do that there, right? I feel like it would also be a good idea to get rid of the $account argument. Plugins that need this can inject the current_user service instead.

DieterHolvoet → created an issue.

I rebased against 3.0-rc13.

DieterHolvoet → changed the visibility of the branch 3270626-taxonomy-integration to active.

DieterHolvoet → changed the visibility of the branch 3270626-taxonomy-integration to active.

DieterHolvoet → changed the visibility of the branch 3270626-taxonomy-integration to active.

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

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

The latest state of the MR doesn't fix the issue for me. When saving a translation, I get 'Non-translatable field elements can only be changed when updating the current revision.'. This issue is not present when I apply the patch from #41.

In both cases I do have another issue: I still get the 'Invalid translation language (und) specified' error, unless I add the following snippet, in that case everything works as it should.

-                $target_entity->addTranslation($active_langcode, $target_entity->toArray());
+                $target_entity_translation = $target_entity->addTranslation($active_langcode, $target_entity->toArray());
+                if (\Drupal::hasService('content_translation.manager')) {
+                  $manager = \Drupal::service('content_translation.manager');
+                  $manager->getTranslationMetadata($target_entity_translation)->setSource($target_entity->language()->getId());
+                }

I have seen this fix/workaround be necessary for multiple other modules dealing with translations, so I'll add it here as well.

• #2593509: Set source language in content entity source →
• 🐛 "Invalid translation language (und) specified" when creating/updating translations Needs review
• 🐛 Node menu link translation breaks content translation Needs review
• #3118218: "Invalid translation language (und) specified" when generating translated content →

It's probably an issue with content_translation, but I haven't been able to figure that out.

Pipeline is fixed!

DieterHolvoet → created an issue.

It still fails randomly, depending on how fast you type, and the Error fetching data: DOMException: The operation was aborted errors still appear in the console. I feel like there's still something else going wrong, other than the configured timeout.

Never mind, 150 still fails sometimes. 250 seems to be a good default.

150 seems to work on both as well. I'll go for that value on my website right now, but maybe it would be best to make this configurable (or at least alterable through some hook) in order to improve responsiveness on faster sites.

Okay, I noticed 50 works great on my local environment, but it doesn't work on production. 250 works on both. This is probably a performance thing, depending on how long page loads take. In that case 250 is probably a safer bet - or maybe something in between. I'll try out some different values on production.

Right, I assumed 'admin' and 'editor' reference the user role, but they reference the menu name. We use a different menu in the admin toolbar for editors using the Admin Menu Swap module → .

I started a MR with a version of the patch rebased on 2.x and with some improved readability.

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

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

DieterHolvoet → created an issue.

I added a 'Remaining tasks' section to the issue summary in an attempt to create an overview what still needs to be done here. I think the biggest thing would be to create a (configurable) list of base urls and use that both in backend and in frontend to strip entered urls, instead of only using the current base url (global $base_url, which is the same as \Drupal::request()->getSchemeAndHttpHost()).