Problem/Motivation

This may be fixable downstream, but since it affects two contrib modules, I'm opening upstream first in case it is a regression.

#3311365: Use PHP attributes for route discovery added attribute route discovery. The code assumes that all attributes are strings, but contrib modules JSON:API Resources and JSON:API Extras both use arrays because they impersonate the core JSON:API. My understanding is that these modules need to override core JSON:API to provide their functionality, but I don't know a lot about the details.

As a result, whenever the modules get loaded, there is a [warning] Array to string conversion AttributeRouteDiscovery.php:41 in the logs.

I noticed this because my functional tests started failing because I had failOnWarning=true, and this array to string conversion warning failed the tests.

Steps to reproduce

Enable JSON:API Extras and JSON:API Resources.

✗ ddev drush cr
 [warning] Array to string conversion AttributeRouteDiscovery.php:41
 [warning] Array to string conversion AttributeRouteDiscovery.php:41

One warning per module.

Proposed resolution

Not sure whose responsibility this is.

Issue fork drupal-3600697

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ptmkenny created an issue. See original summary.

ptmkenny’s picture

Title: Attribute route discovery does not handle arrays for JSON:API contrib modules » 11.4.0-rc1: Attribute route discovery does not handle arrays for JSON:API contrib modules
longwave’s picture

Priority: Normal » Critical
longwave’s picture

Priority: Critical » Major

Oh it's only a warning, still we should fix it for 11.4. Contrib shouldn't need to change anything here.

quietone’s picture

Title: 11.4.0-rc1: Attribute route discovery does not handle arrays for JSON:API contrib modules » Attribute route discovery does not handle arrays for JSON:API contrib modules
Issue tags: +11.4.0 release priority

kieran.cott made their first commit to this issue’s fork.

kieran.cott’s picture

Assigned: Unassigned » kieran.cott
kieran.cott’s picture

kieran.cott’s picture

I've also been able to reproduce this issue.

It looks like AttributeRouteDiscovery::collectRoutes() currently assumes every namespace value is a string path, $directory .= '/Controller';.

However, core already supports namespace values being either a string or an array of PSR-4 base directories. DrupalKernel::classLoaderAddMultiplePsr4() explicitly documents and handles both shapes when registering container.namespaces.

So I think what happens is that when the route builder calls the new router.builder.attributes service with the full container.namespaces service, any namespace mapped to multiple paths causes the scanner to append '/Controller' to an array. That then triggers the warning and effectively checks Array/Controller, so attribute route discovery is skipped for that namespace.

I'm working on a fix to AttributeRouteDiscovery that normalises each namespace value to one or more directories before appending /Controller and scanning. I'll see if I can add a test covering a namespace value passed as an array of directories as well.

kieran.cott’s picture

Status: Active » Needs review

I've opened an MR 16058 to implement the proposed fix in #9.

I've closed the erroneous MR 16057 since the issue fork should have been created from 11.4.x rather than main.

Interested in your thoughts and suggestions.

guillaumeg’s picture

I can confirm the patch gets rid of:
[warning] Array to string conversion AttributeRouteDiscovery.php:41
when clearing the Drupal cache with the contrib module jsonapi_resources enabled, for example.

longwave’s picture

Status: Needs review » Needs work

We need to fix this in main as well, but there is a merge conflict likely due to #3584793: Use PHP attributes for form route discovery - can you raise a separate MR against main please?

kieran.cott’s picture

Status: Needs work » Needs review

Thanks @longwave - I've reopened MR 16057 and brought this up to date against main.

catch’s picture

Status: Needs review » Reviewed & tested by the community

The MR looks good to me - just iterating over the potential array. I was going to ask for an inline comment for the nested foreach but @longwave pointed out that the docblock change already points out we could be dealing with an array of directories, so that's fine too.

  • longwave committed 620cbeb9 on 11.4.x
    fix: #3600697 Attribute route discovery does not handle arrays for JSON:...

  • longwave committed a30f11b1 on 11.x
    fix: #3600697 Attribute route discovery does not handle arrays for JSON:...

  • longwave committed 97a0dbb3 on main
    fix: #3600697 Attribute route discovery does not handle arrays for JSON:...
longwave’s picture

Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 97a0dbb3324 to main and a30f11b1c67 to 11.x. Thanks!

Committed and pushed 620cbeb9637 to 11.4.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.