Project Browser: TypeError Unsupported operand types int + string in Drupal\admin_toolbar_tools

Hi @maxilein,
Steps to Reproduce (Unable to Replicate the Issue):
1. Upgraded Drupal core from 11.2.2 to 11.2.3.
2. Ran database updates using: drush updb
3. The update ran successfully without any errors. Unable to reproduce the reported issue.

Could you please provide more details or specific steps so I can attempt to reproduce the issue on my setup?

Hi @maxilein,

Here is a quick patch to fix those errors while running updates.

To reproduce this you must be using latest Project browser.

Project Browser changed how it's keying things, it seems, so the $key we iterate is no longer numeric.

          foreach ($project_browser_enabled_sources as $key => $source_id) {
            $links['project_browser.browse.' . $source_id] = [
              'route_name' => 'project_browser.browse',
              'parent' => 'system.modules_list',
              // Menu items are ordered by the enabled sources.
              'weight' => -10 + $key,
              ....

I've confirmed that the order the user places them in matches the order they end up in the config object in, so I would say it might make sense to just iterate based on the array positions. However, the current iteration of the patch just does (int) $key which is always a string and would make them all have the same weight.

I have opened an MR based on the patch, but with some different intelligence for handling the weighting (which was the real issue in the first place).

Thank you for the ground work, the first patch seems maybe a bit overly defensive with checks that are not strictly related to the problem and then to prevent the error it casts the string value to int making it zero, so in that case the problem is not really solved because the original ordering of Project Browser links is not preserved (because all weights are now 0).

Here is a simple patch that uses a counter to preserve the original ordering of Project Browser links to address the issue.

Sorry Im late to the party I see chrisfromredfin had already figured this out, I made this patch yesterday and just remembered I had not posted it yet. I also noticed the enormous diff on the MR, we only need to change a handful of lines so maybe something went wrong there.

Thanks for reporting and clarifying the error source and task, I am updating the Issue Summary.

It does look like the GitLab MR is pretty big, so maybe the Status should be "Needs work"?

Yeah, I think the huge diffstat is a result of formatting it for coding standards, because I thought we needed to (Pipeline is failing for phpcs).

I think we should apply the patch from 10 into !169 and push that and re-mark Needs Review. Setting back to Needs Work; hopefully ressa can do that work and then re-mark NR?

Thanks everyone for the great help on this issue!

Quick update of the merge request, as an attempt to keep this moving along with testing and reviews:

• Reverted the last commit, which sort of broke the coding standards of the file.
• Fixed the remaining PHPCS and PHPSTAN errors manually.

I haven't really had the time to test this myself manually though, with the Project Browser module installed and all...

So testing, comments and feedback would be greatly appreciated!

Back to Needs review!

Thanks in advance!

Successfully tested on Drupal 11.2.5 and Admin Toolbar 3.6.2.

I tested this MR on Drupal 10, and it seems to work there too. Thanks a lot.

Thanks again everyone!

This issue was really easy to reproduce:
Install admin_toolbar_tools 3.x (3.6.2, for example) and project_browser 2.1.x (2.1.1, for example).... The error described in the IS would immediately occur.

The initial issue #3506824: Compatibility with Project Browser was fixed to support project_browser 2.0.x which had the following config structure: an array of source ID values
https://git.drupalcode.org/project/project_browser/-/blob/2.0.0-beta1/co...

enabled_sources:
  - drupalorg_jsonapi
  - recipes

and now with project_browser 2.1.x the config is an associative array keyed by source IDs
https://git.drupalcode.org/project/project_browser/-/blob/2.1.x/config/i...

enabled_sources:
  drupalorg_jsonapi: []
  recipes: []

Change introduced in:
https://git.drupalcode.org/project/project_browser/-/commit/2e70410ced6e...
Corresponding to #3533955: Make source plugins support stored configuration options

I tried the previous merge request which indeed fixed the error, but the links under the "Extend" menu (/admin/modules) would disappear.... defeating the purpose of having this code in the module in the first place.

I have created a new merge request MR !185 which not only fixes the error but also restores the initial behavior of the extra links logic by adding all the enabled project browser sources extra links under the "Extend" menu item, see screenshot below:

This patch implies dropping support for earlier versions of Project Browser, in other words, users would have to upgrade and use the latest versions: 2.1.x.
Therefore a conflict constraint was added to module's composer.json file as an attempt to prevent users from using Admin Toolbar's next release with any version of Project Browser lower than 2.1.0, see:

  "conflict": {
    "drupal/project_browser": "<2.1.0"
  }

 
Could you please help testing and reviewing MR !185?

Could you please also review the conflict section? I'm not sure how this could really be tested without merging the changes in the module, since composer constraints and requirements are usually evaluated before patches are applied...
I checked in other contrib modules, for example devel, copied the conflict section and adapted the version compatibility constraint.

I've done some quick tests myself and the logic seems to work exactly as it did for previous Project Browser versions:

• Enabled source plugins links are ordered by their weight.
• Updating the Project Browser config form (/admin/config/development/project_browser) should immediately reflect the changes in the menu.

 
Moving issue back to Needs review as an attempt to get more testing feedback and reviews on this new suggested patch.

Feel free to let us know if you encounter any issues with the suggested merge request or if you have any questions or concerns on any aspects of this comment or this ticket in general, we would certainly try answering as soon as possible.
Thanks in advance!

Since this feature keeps breaking with changes to Project Browser, I thought we might need an automated integration Functional test that would help systematically testing the integration with the evolution of core, admin_toolbar and project_browser.

Added a basic Functional tests class to check the links added by admin_toolbar_tools for project_browser are displayed in the expected order under the 'Extend' menu link.

Hopefully, this should help preventing these types of issues from repeating over and over in the future.

Reviews, feedback and comments are welcome.
Thanks in advance!

Been dealing with this issue for about 2 months after trialing CMS but not high priority as can get by without tools operating. Originally thought it was a download issue as download failed. Anyhow thought I would try turning off what ever the button is in project browser config with the result I have the following error.

The website encountered an unexpected error. Try again later.

TypeError: Unsupported operand types: int + string in Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks->getDerivativeDefinitions() (line 749 of modules/contrib/admin_toolbar/admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php).
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives() (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 127)
Drupal\Core\Menu\MenuLinkManager->getDefinitions() (Line: 152)
Drupal\Core\Menu\MenuLinkManager->rebuild() (Line: 113)
admin_toolbar_tools_project_browser_settings_submit()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 144)
Drupal\autosave_form\Form\AutosaveFormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 97)
Drupal\autosave_form\Form\AutosaveFormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 715)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Could anyone encountering this issue please try the patch from MR !185?
(see #21 above for more details)

Download the patch in plain diff format at the following URL:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/185.diff
and apply it to your project as for any patch for contrib modules.

We're waiting on additional feedback and reviews before getting these changes added to the module....

Thanks in advance!

I've this issue with 3.6.2 and Drupal 11.3.0-rc1. With the patch from MR!185 the issue is gone --- perfect!

Thanks a lot Thomas (@drupatz) for your positive feedback and confirmation the patch fixed the issue! 🙏

Following your confirmation, I added a few minor changes to the doc comments blocks of the new Functional tests file and did another round of testing manually locally.
The links added by the module worked great with the correct order in the admin toolbar menu. 👌

Since everything seemed to work fine and the automated tests or jobs were all passing 🟢, I went ahead and merged the changes above at #28. 🥳

Note the 'conflict' constraint in the composer.json file seems to work fine as well, since composer jobs for certain core versions (previous major, for example) appear to be unable to find a compatible version (greater than 2.1.0) package and thus, required a special case to conditionally skip the Functional test. 👍

The changes should be released soon with the upcoming 3.6.3...
This is definitely an important issue we're glad to cross off our list... just a few more to go 😅

Since I don't see anything else remaining or any follow-up task for this ticket for the moment, it is probably safe to consider it as Fixed for now.

The added Functional tests should allow us to detect any breaking changes with the different versions of the Project Browser module and be able to address them quicker or even anticipate them earlier.
This should make the two modules integration more robust and hopefully, decrease the amount of maintenance work on our side or support requests we get to fix this particular feature 😅🤞

In any case, feel free to let us know if you still encounter any issues with the two modules, or would have any questions on any of the most recent code changes, we would certainly be glad to help! 😊

Once again, thanks a lot to everyone for the great help keeping the Admin Toolbar Tools module well maintained! 🙏
Cheers!