The [current-page:title] is broken on some routes

Created on 23 May 2017, over 7 years ago
Updated 19 March 2024, 6 months ago

The token [current-page:title] uses the title resolver service which only looks for title in the route definition. If it won't find any value it will return NULL. You can see this in the node edit form where the page title is set within the form.

Drupal core will take this value and put it into the head_title variable in the template_preprocess_html which will then contain the title and namekeys where title being the page title(from route or from the page render array) and the name being the site name. These variables are then(based on theme, but by default) joined by pipe in the html.html.twig template.

The metatag module overrides the title html tag, if it is set to do so, in the metatag_preprocess_html where it removes the name value and replaces the title value with rendered token.

Since the token for the current page title won't get the value it will be empty and in case of [current-page:title] | [site:name] it will result into | My Site.

Without metatag altering the title, the original one will be used and a proper value is displayed. I think since metatag causes this issue it is this module's responsibility to create a fix for this behavior.

Since the html tags are being attached in the html preprocesor and the real value of the page title can be accessed only in there, it should be passed onto the encapsulated metatag functions and a special handling of the aforementioned token should take place beforehand.

The attached patch solves this with one drawback - it regenerates all tokens and ignores the static cache of previously generated metatag data.

πŸ› Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡©πŸ‡ͺGermany Grevil

    This is still happening in 2.0.x!

  • Open on Drupal.org β†’
    Core: 9.5.5 + Environment: PHP 7.3 & MySQL 5.7
    last update 6 months ago
    Not currently mergeable.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 8
    last update 6 months ago
    126 pass, 4 fail
  • Pipeline finished with Failed
    6 months ago
    Total: 285s
    #123197
  • Pipeline finished with Failed
    6 months ago
    Total: 295s
    #123199
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 8
    last update 6 months ago
    126 pass, 4 fail
  • πŸ‡©πŸ‡ͺGermany Grevil

    Ok, it seems that "hook_metatags_attachments_alter()" isn't working as expected anymore, or the implementation in the "metatag_test_integration" test module needs a fix.

    Maybe someone else could take a look? Otherwise, this seems to work as intended! :)

  • Pipeline finished with Failed
    6 months ago
    Total: 328s
    #123231
  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA

    I think we need to expand the test coverage to confirm the problem.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    This should be considered with Major priority. I would tend to say: The current-page:title is the most important part in the browser windows tab and search engine preview of a website and so-to-say THE meta tag. Descriptions of the Priority β†’ and Status β†’ values can be found in the Drupal project issues β†’ documentation.

    I think we need to expand the test coverage

    While I agree with test coverage, I really see a deep impact here in every page presented, if this project is used. Are we sure to not merge it at least on the dev branch for those who are willing to take the risk at their side?

    Ok, it seems that "hook_metatags_attachments_alter()" isn't working as expected anymore...

    Yeah ... average users may did not even recognize it soon, as long as they do not run some website test calls, because this issue seem to appear silently of a sudden and you start to think: when does this happened and how long is the website in this state? Also common in such cases like this long holding issue is to put a note on the project page linking to this issue here, so that users can chime in and help on test coverage and anything else to be needed.

    I would chime in but I would have to go from ground zero into metatag's code base and my time is limited atm because my burn-out from last year did teach me lessons. But if I can make some time free and nothing moved forward here I would surely come back on this. But not before end of this year, I guess.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    Hiding outdated patch to focus eyes on @grevil's MR. Also for those who need this and are on dev branch anyway you can test this issue by adding this to the repositories in your composer.json:

      "repositories": [
        {
          "type": "vcs",
          "url": "https://git.drupalcode.org/issue/metatag-2880604.git"
        },
    ...
      ],

    And then run: composer require drupal/metatag:dev-2880604-the-current-pagetitle-is

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    Oh, that's why. We need to track MR against 2.1.x dev now...

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Just had a look into some projects, where we're using
    [current-page:title] | [site:name]
    as global pattern.

    Following the steps and for example visiting /node/add/page I'm not able to reproduce the issue!
    The page title is Web-/Landingpage erstellen | My Drupal project as expected.

    Also I didn't see any recent reports of this since #21
    Maybe it was caused by a conflict with another module or a solved bug?

    I can't really remember why @Grevil started working on this, maybe we had the issue somewhere? I don't know, but currently I can't reproduce it.

    @dqd could you perhaps try to reproduce it in a vanilla Drupal installation or at simplytest.me using the steps from above?

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    @dqd could you perhaps try to reproduce it in a vanilla Drupal installation or at simplytest.me using the steps from above?

    As the title says "on some routes" ... This issue is not about vanilla Drupal install issues with metatag. We need to keep track on the reports because it is not warrantied that this is fixed. Maybe only hidden in your case.

    @anybody: Sorry, I hope you do not feel offended but please do not let us clutter the issue now by one report who cannot reproduce the issue no more in running projects. And this issue is not about vanilla Drupal install but about how things can get into the way so that current-page:title is empty if called from the wrong angle. It is reported why this can happen and that it has its corner cases. And it has been explained why from top to bottom briefly. I pinged @Grevil on Slack an hour ago and he would chime in later if possible, which will possibly be the best bet here to go on before keeping people busy.

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    @dqd all good, but we need clear steps to reproduce this and I can't see them or reproduce it.

    My manual test and feedback regarding node create / edit came from this sentence in the issue summary:

    The token [current-page:title] uses the title resolver service which only looks for title in the route definition. If it won't find any value it will return NULL. You can see this in the node edit form where the page title is set within the form.

    Maybe that was too simple, yes.

    Still I think this definitely needs clear steps to reproduce (manually), in the issue summary, to ensure the tests are correctly based on that. That was my point. We can't even be sure the test here is complete / correct yet. Any things may have changed since 2017.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    I edited my comment in the time you answered quickly... So, maybe there is some context confusion now. Again, steps to reproduce are there and testing on vanilla install is meaningless regarding the issue description and the comments following. Latest report about issues on this are from 2021, not from 2017.

    Trying to reproduce an issue on a vanille Drupal install which occurs here but not there is like hunting the white rabbit. Again, it would be better and more time saving if people who worked on the latest fixes can have a quick look if the way the token gets fetched is still the issue. If this is not possible for other reasons, no problem, I can have a briefly chat with Damien later if time is there.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    Wow THAT! was quick @Grevil +1 Awesome!

    I will call out for user tests/reviews. If the issue disappears for them with this MR, then it obviously is still worth merging. +1 @Grevil

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡©πŸ‡ͺGermany Grevil

    Changed the target branch and rebased the fork on current 2.1.x!

    @dqd Not really in the picture anymore, with this issue. As @Anybody already mentioned, we are not using the current MR as a patch in any of our projects, but I am very certain the described issue is still a thing, as I am seeing it in our dev environment from time to time.

    So feel free to test the MR & adjust the tests to get this fixed ASAP! Thanks for continuing here! πŸ˜πŸ‘

  • πŸ‡©πŸ‡ͺGermany Grevil

    I will call out for user tests/reviews. If the issue disappears for them with this MR, then it obviously is still worth merging.

    Great stuff! Glad my past self could help! πŸŽ‰

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Thanks @grevil and @dqd. Thinking about this for a while, we might have had the issues when we worked on https://www.drupal.org/project/homebox β†’ 3.x branch. I think that might have been the place where we ran into this and I asked @Grevil to have a look. Next time, we should also add more context, where we ran into it and which steps are needed for reproduction. :)

  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA

    FWIW the test failure is a false negative.

    Too bad it doesn't seem possible to do a tests-only change anymore using merge requests, without creating another MR with just the tests changes.

    The attached patch solves this with one drawback - it regenerates all tokens and ignores the static cache of previously generated metatag data.

    That's two drawbacks ;-)

    The first drawback is performance-related. Might the second issue might cause problems for people with complicated setups?

    It might be worth modifying this to not assume $variables['head_title']['title'] exists.

    Also, could it be reworked to only run metatag_get_tags_from_route() if the title existed?

    Lastly, the comment on metatag_handle_real_page_title_token() is a bit redundant.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    FWIW the test failure is a false negative.

    Yeah, that's what I guessed ...

    Might the second issue might cause problems for people with complicated setups?

    Well any even minor issue cause problems for people with complicated setups. :) Just kidding. TBH, if these concerns only belong on "save" when editing Metatag setup and not on each page load even if all caches are enabled, I think this is tolerable. Setting up MetaTag isn't a minor setup.

    Also, could it be reworked to only run metatag_get_tags_from_route() if the title existed?

    I can try to have a look when I get a free minute and some sleep.

    But I can at least report from 2 folks I spoke with and told them to please tryout the latest MR on Drupal 11 projects, that the page title finally came back on their projects after multiple cache clears and some reloads. Yay.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    WAIT! I have to find out if they have updated from 2.1.x to this MR or from 2.0.x dev from project page to this MR. Because 2.1.x is not listed on the project page and other things on 2.1.x could have fixed parts of it.

  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA

    fwiw the only change in 2.1.x is migration-related.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    THX! good to know, so it doesn't matter actually. Still no response yet from the other side, but I will test myself next days...

Production build 0.71.5 2024