🇧🇪Belgium @jelle_s

I believe the patches were created to show what I said in #76 as a proof of concept. As long as there's no consensus on what to do between the suggestions of #75 and #76 I guess it makes sense not to clutter the MR with all the different ideas?

I agree with #75, but since Drupal already uses symfony/http-foundation, we could possibly use
\Symfony\Component\HttpFoundation\ Symfony\Component\HttpFoundation::parseQuery()?

@markie If this project is still actively maintained, could you mark one of the branches as supported? Our update status marks this module as unsupported right now

smustgrave → credited jelle_s → .

Rerolled patch.

el7cosmos → credited Jelle_S → .

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

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

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?

Rerolled #88

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

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

Reroll for 10.3.x

Rerolled patch for Drupal 10.3.x

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.

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.

Jelle_S → created an issue.

xjm → credited Jelle_S → .

Thanks!

Fixed, thanks for the patch!

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

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

The Drupal status page reports it as a critical issue:

Jelle_S → created an issue. See original summary → .

Jelle_S → created an issue.

Jelle_S → created an issue.

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.

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

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.

Removed the extra whitespace at the end of the file

@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.

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.

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...

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

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

FYI: This introduced a bug. See 🐛 Admin page access denied even when access is given to child items RTBC

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

FYI: This introduced a bug. See 🐛 Admin page access denied even when access is given to child items RTBC

Jelle_S → created an issue.

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.

Jelle_S → created an issue.

Reroll of #96

Patch for getRegistrantQty since we ran in to that problem.

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

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

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.

Reroll of #39 for Drupal 10.1.x

@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

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.

Updated patch

This patch should fix it.

Reroll of #89

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

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

Reroll against 10.1.x

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);

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.

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

Seems to work as expected

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

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

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

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"

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.

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!

Jelle_S → created an issue.

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.

Can confirm this works as well

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

Still not giving up on this.

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

I seem to have missed a couple.

@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.

Let's try this again

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?

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).

Thank you for your feedback. I created #3364210: Wrong recommended version for LinkIt module → to have it checked.

Jelle_S → created an issue.

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).

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.

Looks good to me.

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

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

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

Jelle_S → created an issue.

Reroll of #5.

@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.