Problem/Motivation

The constant exists for this purpose but is not being used.

Steps to reproduce

N/A

Proposed resolution

Replace the string with the constant.

Remaining tasks

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#16 3538669-nr-bot_thf1bpfz.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3538669

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:

About issue forks

Comments

nicxvan created an issue. See original summary.

julio_retkwa made their first commit to this issue’s fork.

julio_retkwa’s picture

julio_retkwa
Primary language Portuguese, Brazil
Location Santa Catarina
commented

I'll take a look on this

julio_retkwa opened merge request !12910

julio_retkwa’s picture

julio_retkwa
Primary language Portuguese, Brazil
Location Santa Catarina
commented

does that looks like as it should ?

julio_retkwa’s picture

julio_retkwa
Primary language Portuguese, Brazil
Location Santa Catarina
commented
Status: Active » Needs review

nicxvan changed the visibility of the branch 3538669-controllerresolver-should-use to hidden.

nicxvan changed the visibility of the branch 3538669-controllerresolver-should-use to active.

nicxvan’s picture

nicxvan commented

I think it's supposed to be on the branch that was created, not the 11.x.

I'm not sure if that actually causes a problem or not though.

mstrelan’s picture

mstrelan commented

I think we should expand the scope to change this everywhere _controller is used.

The constant is currently used in these places:

\Drupal\jsonapi\Revisions\ResourceVersionRouteEnhancer::enhance
\Drupal\jsonapi\Routing\Routes
\Drupal\Tests\jsonapi\Unit\Routing\RoutesTest

Should we update in the following places too:

\Drupal\Core\Controller\ControllerResolver
\Drupal\Core\Controller\ControllerResolverInterface (docs)
\Drupal\Core\Entity\Enhancer\EntityRouteEnhancer
\Drupal\Core\Entity\EntityResolverManager
\Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider
\Drupal\Core\Routing\Enhancer\FormRouteEnhancer
... probably some other places too

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Let's take care of all of them and move to the branch the issue created.

julio_retkwa opened merge request !12914

neerajsingh made their first commit to this issue’s fork.

neerajsingh opened merge request !12940

neerajsingh’s picture

neerajsingh
Primary language English
Location IN 🇮🇳
commented
Status: Needs work » Needs review

Raised PR with fixes in the following files:

  1. core/lib/Drupal/Core/Controller/ControllerResolverInterface.php
  2. core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
  3. core/lib/Drupal/Core/Entity/EntityResolverManager.php
  4. core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
  5. core/lib/Drupal/Core/Routing/Enhancer/FormRouteEnhancer.php
  6. core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3538669-nr-bot_thf1bpfz.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

luismagr made their first commit to this issue’s fork.

luismagr’s picture

luismagr commented
Status: Needs work » Needs review

I've updated the fork with upstream. Let see what the bot says now

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Appears all instances of ->attributes->get('_controller') has been replaced with the constant.

catch closed merge request !12940

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

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

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

Maintainers, please credit people who helped resolve this issue.

  • catch committed 34fc7019 on 11.x 
    Issue #3538669 by nicxvan, julio_retkwa, mstrelan, neerajsingh:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.