Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix drupal_set_title() changing breadcrumb titles #100

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

darrenoh
Copy link
Contributor

No description provided.

Copy link
Collaborator

@mglaman mglaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a way to reproduce this? I wonder if we could make RetrofitTitleResolver aware of the RequestStack instead. Then we can use SplObjectStorage to store the title based on objects.

That is how we handled it in Commerce for request-specific things https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/store/src/CurrentStore.php?ref_type=heads

@darrenoh
Copy link
Contributor Author

I experienced this with Ctools on export UI page callbacks. Each callback ran drupal_set_title(), so when the breadcrumb was generated every link had the same title.

Object storage sounds good, but I don't know what RequestStack is.

@mglaman
Copy link
Collaborator

mglaman commented Nov 17, 2023

The RequestStack is how \Drupal::request() works, it tracks all the requests being handled. The RetrofitTitleResolver would actually look like the path.current service I think: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Core/Path/CurrentPathStack.php?ref_type=heads

@darrenoh darrenoh force-pushed the fix-set_title branch 2 times, most recently from ef55f74 to 94f2b2d Compare November 20, 2023 21:13
Copy link
Collaborator

@mglaman mglaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines +49 to +52
$requestStack = $this->container->get('request_stack');
assert($requestStack instanceof RequestStack);
$request = $requestStack->getCurrentRequest();
assert($request instanceof Request);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I'm surprised this works without pushing a request to the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that pathInfo is not set when a request is created, so it never matched the request from the request stack.

@mglaman mglaman enabled auto-merge (squash) November 23, 2023 19:24
@mglaman mglaman merged commit a2dc737 into retrofit-drupal:main Nov 23, 2023
3 checks passed
@darrenoh darrenoh deleted the fix-set_title branch November 23, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants