Antwerp, Belgium
Account created on 8 June 2010, about 14 years ago
#

Merge Requests

Recent comments

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

And the patch for those that want to use it in composer.patches.json

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Jelle_S β†’ made their first commit to this issue’s fork.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I've not been able to test yet, but I was wondering what this patch/MR would do if there are these wildcard redirects:

a-path/* -> redirect1.com
a-path/b-path/* -> redirect2.com

and you would navigate to a-path/b-path/c-path, which redirect would it use?

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Just to be sure, for those that might still need it, here is the reroll for 10.3.x

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Here's the patch from that MR for those that wish to use it in their composer.json file

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Rerolled patch for Drupal 10.3.x

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

After some digging around, I found πŸ“Œ Replace RequestCloseSubscriber with needs_destruction tag on ModuleHandler Fixed , where Drupal core's ModuleHandler got the needs_destruction service tag. It seems that since HookEventDispatcherModuleHandler decorates that service, it also has the needs_destruction tag, meaning that it should probably also implement Drupal\Core\DestructableInterface and the destruct method. That interface has existed since Drupal 8.0.0, so it's safe to implement without having any BC breaks.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Problem with that is that the dummy block entity doesn't have the third party settings. Before the change in πŸ› Settings no longer stored as third party settings Fixed was introduced, it worked with menu block (plugins) that were referenced by the block_field module. I added some extra lines to the patch that makes it work again, but I'm not sure this is the behavior we want by default? It seems to work for our use case tho.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Jelle_S β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Fixed, thanks for the patch!

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I'm assuming the fix in πŸ› Admin page access denied even when access is given to child items RTBC fixes this issue as well? If so, you can close this as duplicate. Fixes like this will be committed to 11.x first and then backported to 10.2.x

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Yeah that's the problem, since the project itself has no supported releases:

The Drupal status page reports it as a critical issue:

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

For anyone experiencing this issue: the patch/merge request over at πŸ› Admin page access denied even when access is given to child items RTBC should fix it for you.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Sorry, I didn't see the big green "Get push access button"... Thanks @taraskorpach for adding the latest patch to the MR :)

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Still no access to the MR, but here's the above patch with tests included, and one patch with only the tests to prove they fail.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Removed the extra whitespace at the end of the file

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

@taraskorpach I don't have push access to your MR, but I did find a fix for the problem described above. Not sure how to write a test for this exactly, but in the mean time people experiencing this issue can use the patch attached.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

After some further digging:

The scheduler module adds the view linked above, which creates a menu link item with entity.taxonomy_vocabulary.collection as its parent. So if you follow the steps to reproduce, you create a user that does have access to entity.taxonomy_vocabulary.collection, but does not have access to its child created by the scheduler module: views_view:views.scheduler_scheduled_taxonomy_term.overview

I'm still trying to wrap my head around the correct implementation details, but basically: once an element in the tree is found that is accessible, and is an actual page, not just an overview of its sublinks, the access check should pass, which it doesn't right now.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

It seems to have something to do with this view in scheduler, but I can't figure out what's going wrong:
https://git.drupalcode.org/project/scheduler/-/blob/2.0.1/config/optiona...

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I've managed to reproduce it on a clean Drupal 10.2.1 install, with the contrib Scheduler module. Unfortunately, in this case, this patch does not seem to work, so there's more going on. Here are the steps to reproduce:

composer create-project drupal/recommended-project drupal.local
cd drupal.local
composer require drush/drush
vendor/bin/drush site:install
vendor/bin/drush user:create Jelle
vendor/bin/drush user:role:add content_editor Jelle
vendor/bin/drush role:perm:add content_editor "access taxonomy overview"

# Login as user 2 and check the toolbar, "Structure" should be there

composer require drupal/scheduler
vendor/bin/drush en scheduler

# Login as user 2 and check the toolbar, "Structure" is gone

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

No, that tackles a different issue. I'll have a look at creating an MR if I find the time today.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

My bad! I linked the wrong issue, the code definitely is already in core. Fixed that part.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
  1. +++ b/src/Plugin/Validation/Constraint/UserMailUniqueValidator.php
    @@ -36,9 +24,7 @@ class UserMailUniqueValidator extends UniqueFieldValueValidator implements Conta
    +  public function __construct(protected AuthmapInterface $authmap, protected OpenidConnectRealmManagerInterface $realm_manager,protected EntityFieldManagerInterface $entityFieldManager, protected EntityTypeManagerInterface $entityTypeManager) {
    

    Code style: space after $realm_manager,
    Code style: rename $realm_manager to $realmManager
    Drupal uses snake_case for method arguments, but camelCase for properties; I believe the fact you're defining a property here has precedence over the fact that it's a constructor argument.

  2. +++ b/src/Plugin/Validation/Constraint/UserMailUniqueValidator.php
    @@ -68,7 +57,7 @@ class UserMailUniqueValidator extends UniqueFieldValueValidator implements Conta
    +        if ($this->realm_manager->hasDefinition($plugin_id)) {
    

    Revert this change based on previous remark.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Jelle_S β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Patch for getRegistrantQty since we ran in to that problem.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

@maskedjellybean I don't know the technical details, but according to #4 it's a performance optimization

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

RE #5: Has this been tested? Is it possible to revert to 1.x without issues?

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Wrapping it would look something like this:

mymodule.services.yml

mymodule.tree_storage_wrapper:
    class: Drupal\mymodule\TreeStorageWrapper
    arguments: ['@menu.tree_storage']
namespace Drupal\mymodule;

use \Drupal\Core\Menu\MenuTreeStorageInterface;

class TreeStorageWrapper {

  protected $menuTreeStorage;

  public function __construct(MenuTreeStorageInterface $menu_tree_storage) {
    $this->menuTreeStorage = $menu_tree_storage;
  }

  // Define the methods you need to use the menu tree storage for.
}

This is untested code, but roughly how it would be done.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

@Luke.Leber best I can tell is you can create your own service, inject it as a parameter in that service through your services.yml and call it from there, basically wrapping the non-public service into a public service

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I believe this is the same issue I'm experiencing:
When only one format is selected for a field (and that format has a wysiwyg profile), the format select dropdown is not shown under a textarea. The wysiwyg module seems to expect it to always be there.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Updated patch

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

This patch should fix it.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Fixed the call to the dispatch method. I kept the usage of the event name in there for BC.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I'm getting

TypeError: Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch(): Argument #1 ($event) must be of type object, string given, called in [...]/web/modules/contrib/oidc_menu/src/Plugin/Block/OpenidConnectMenuBlock.php on line 284 in Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (regel 89 van [...]/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php).

Patch incoming

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

In case this helps someone else: Since we only use the load method of the MenuTreeStorage class, we were able to replace it with the MenuLinkManager.

-    $container->get('menu.tree_storage'),
+    $container->get('plugin.manager.menu.link'),
-    $menu_link = $this->menuTreeStorage->load($id);
+    $menu_link = $this->menuLinkManager->getDefinition($id, FALSE);
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

The problem occurs when trying to inject the service in a plugin like for example a block. This is because of the public static function create pattern I assume, since that technically doesn't count as "injectable" I guess.

Example code:

class CrisisCommunicationBlock extends DgCrisisCrisisCommunicationBlock {

  // ...

  /**
   * Class constructor.
   *
   * @param array $configuration
   *   The block configuration.
   * @param string $plugin_id
   *   The plugin id.
   * @param array $plugin_definition
   *   The plugin definition.
   * @param \Drupal\Core\Menu\MenuTreeStorageInterface $menu_tree_storage
   *   The menu tree storage service.
   */
  public function __construct(array $configuration, $plugin_id, array $plugin_definition, MenuTreeStorageInterface $menu_tree_storage) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->menuTreeStorage = $menu_tree_storage;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition): DgCrisisCrisisCommunicationBlock {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('menu.tree_storage'),
    );
  }

This did work on Drupal 9.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Rerolled #140 against 10.1.x. There is no interdiff since the conflicting line wasn't part of the diff.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Seems to work as expected

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

The adminimal theme has a D10 compatible release since 08/08/'23, time for a stable release of the toolbar I'd say ;)

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

RE #23: That is indeed the desired behaviour I would say. The difference there is that the "new" version is a major version change (1.0.x -> 2.0.x) and for the linkit module it's a minor version change (6.0.x -> 6.1.x). In semantic versioning, there should be no BC breaks between minor versions, and (imho) dropping support for a version of Drupal core is a BC break. Either way, the reason Drupal update reports it as a warning is because it's only a minor update and so it should be possible to update to that version

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I've got the same thing happening on 8.x-1.12: it keeps saying 8.x-1.11 is the recommended version?

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Also, the problem we're experiencing is Drupal 9 vs Drupal 10, not 10.0 vs 10.1

Linkit 6.0.x is compatible with both D9 & D10, while 6.1.x is only compatible with D10, even tho it's just a minor version update, which (besides the quirky "recommended release" Drupalism) is the main reason the update module reports it as an "available update"

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

RE #26:

OK, so it thinks it's done the update. Maybe that's a result of enabling the module after updating the .install file, I'm not sure what Drupal does in that situation.

When you install a module from scratch, Drupal trusts hook_install does everything that's necessary, and sets the latest schema version to the latest update hook. It doesn't run any existing update hooks. You'll probably need to create that row in hook_install as well.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Just informing if there is any vision on how and/or if this will be fixed. Our monitoring is reporting this update for a lot of our sites now, cluttering our overview. Thanks!

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Jelle_S β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

RE #14:

I think the biggest challenge is validation. We'd need to be able to make all our validation logic work on the client side somehow.

I'm about 6 years late to reply, but I've been a maintainer of the Clientside Validation β†’ module. Admittedly, I haven't been active on that project for quite some time, others have taken over, but it might me a good starting point/inspiration for the validation of forms on the JS side.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Can confirm this works as well

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

FWIW: We're using the latest MR in a Drupal 9.5.9 website with the Seven theme from contrib. Works as expected.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Still not giving up on this.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Don't worry, I'll get there eventually.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I seem to have missed a couple.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

@dineshkumarbollu I've added the trait everywhere I'm supposed to as far as I'm aware. If you are using the upgrade_status module, make sure to rescan the module after you've applied the patch.

New patch fixes code style issues reported by the testbot.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Let's try this again

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I'm not sure why this is testing against 1.0.x and not 1.x, but I can't seem to add a test for 1.x. I'm guessing one of the maintainers needs to configure something?

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Ok, so based on drumms feedback the infrastructure works as designed. I'm gonna create a core issue since the update status overview page does not take the core compatibility of the modules into account. https://updates.drupal.org/release-history/linkit/current clearly states 6.1.0-rc1 is only compatible with D10, and thus should not be mentioned as recommended update on the overview page of a D9 site imho.

<core_compatibility>^10.1</core_compatibility>

Edit:
Bummer, seems like #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core β†’ has existed for a while now, but no real activity in that issue itself since 2021.

A quick fix could be to move the D10 only support to a 7.x branch, and keep the 6.x branches for D9 compatibility (alternatively, keep the 6.x branches for compatibility with both D9 and D10).

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

This created an other issue on Drupal 9 installations that are on 6.0.0-rc1. It reports an update being available, while it's incompatible with D9:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/linkit ^6.1 -> satisfiable by drupal/linkit[6.1.0-rc1, 6.1.x-dev].
    - drupal/linkit[6.1.0-rc1, ..., 6.1.x-dev] require drupal/core ^10.1 -> found drupal/core[10.1.0-alpha1, 10.1.0-beta1, 10.1.x-dev] but it conflicts with your root composer.json require (^9.2).
πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I've tested the default functionality: logging in with an already mapped account, and with an account that wasn't mapped in authmap yet, both worked as expected. Logout still works as expected too. I haven't tested writing a custom event listener yet, but since the default implementation is an event listener as well, I don't expect any problems there.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Looks good to me.

Nitpic: In a couple of docblocks you wrote "authantication" instead of "authentication".

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

I updated the MR based on some of the review remarks.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Thanks for the review, added my remarks as well. I'll update the MR after your input.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

@Wim Leers in #35: I'm not sure "Local content links" makes sense for non-technical end users. To them "local" could just as well mean geographically. I do agree that it makes more sense than the previous suggestions, and that "Drupal" should be avoided in the UI. Maybe "Site content links"? Just spitballing.

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

RE #18 @Wim is there a core issue for this somewhere, are there any concrete plans?

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

There's a mistake in that commit

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Another reroll

Production build 0.69.0 2024