Add a token for the site logo

This needs a reroll against 11.x-dev now. #168 doesn't apply against 10.1.x for me?!

I rerolled #168

Back to NR

Going to mark it but left 1 comment in the MR for the committer.

FYI, casting to string is a requirement for assertSame. See the link in failing test of #184.

@smustgrave see #189, perhaps that comment can be resolved?

Attached MR!4236 diff as static patch, if anyone needs it!

I am doing triage on the core RTBC queue → .

I read the issue summary. There is a proposed resolution so reviewers know what to look for. However, the remaining tasks show another issue, is this issue supposed to be postponed on that or is a follow up needed?

This is adding tokens so there should be a Change record. This should include examples. I am adding the tag and setting to NW.

I then read the comments, mostly trying to find unanswered questions. Its that @Neograph734 did a very nice job keeping up with the feedback. It is hard to follow but I think everything has been responded to expect for two items.
1) There was concern about emails, #17. I am not sure if that was resolved. Or does this need documentation, including the CR, on how using these new tokens with mail will work.
2) Should be using a site-wide token, #173 - #180. In #176 @Berdir stated the proposed structure wouldn't work and there has been no other response from them on the subject. Has this truly been resolved? If that has been resolved it is worth adding a summary of that discussion in the Issue Summary so that it is clear for a committer.

I then read the MR, I did not do a code review. I spotted some things that need changing which I have commented on in the MR.

I am removing the 'Needs maintainer tag' because I think that intention is covered by 'Needs subsystem maintainer review' and it is not a defined special tag → .
This is long issue in comments as well as time (7 years), thanks everyone for getting the bulk of the work done. Just a few more things to do.

I tried to read through the thread to understand it, and as far I understood for #191 ✨ Add a token for the site logo Needs work point 2:
@Berdir I believe is responding to the proposed suggestion for having seperate token type to which he disaggred, and that is something still not addressed as we see a new token type "site-logo" being introduced in the code.

Also @Berdir expressed for someone to verify if suggestions from #161 are implemented which I verifed have been incorporated and this was also verified in #180.

#191 point 1 (refering #17) this I think will relate to what approach we take with respect to the concern raised around introducing new token type, as in the current solution if we take off the new token type, #17 remain open.

It is a situation in hand, will spend some time on it however some direction around how to address #17 will be helpful, keeping in mind that people in general are not inclined towards introducing a new token type.

Adding patch to update changes as suggested in #191.

Still have a few open TODO items on the issue:

1) Decide on an approach to address concern raised in #17 (possibly without adding new token type). If new token type needed then this need to be discussed and agreed upon before moving forward with this approach.

2) IS to be updated
(leaving this for someone who has more hold on this issue thread, will revisit to update IS if not updated in some time).

#168 patch is not applying with the Drupal 10.1.5 version.
Issue with kernal test changes. Updated the changes and adding the new patch.

Fixed failures in #194.

Finally I gave up the hope that this will ever be finished by anyone (including me). So maybe we should better put our time into implementing a contrib solution?

Maybe in https://www.drupal.org/project/theme_tokens → ?
Seems there are enough people here to do that...

I fixed the feedback in the MR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

The tests were failing because the token hooks moved to a hook class.

Tests are now green again.

There was already an MR for 11.x should continue there

Merged 11.x-dev back into the Merge request !4236