Closed (won't fix)
Project:
Diff
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
6 Apr 2023 at 11:51 UTC
Updated:
18 Dec 2024 at 06:37 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
feyp commentedAttached is a patch against 8.x-1.x-dev.
Comment #3
feyp commentedFound an issue with the bubbling of cache metadata, so new patch and interdiff attached. Also updated the tests to assert correct metadata.
Like the previous patch, this requires PHP 7.4 or later, but I think this is just due to some of the type hinting. Looks like the default testing is currently for PHP 7.3. If needed, I can probably provide an updated patch that is compatible with 7.3 as well. Let me know.
Comment #4
Anonymous (not verified) commentedThanks @FeyP ...
#3 - 3352640-03.patch is working.
Tested versions
Drupal: 10.2.7
diff: 1.7
token: 1.14
php: 8.3
+1 for RTBC
Comment #5
acbramley commentedThank you for your contribution. This issue currently does not meet the Contribution guidelines which are required to get this change committed.
Comment #7
feyp commentedThanks @acbramley! I created an MR against the 2.x branch, that should be all that was missing to meet the contribution guidelines for NR.
One small note on this part of the code in TokenProvider:
This was an empty() check in the original patch for 1.x, because I actually ran into this during development. empty() checks are no longer allowed by the phpstan configuration of this module. Thus I looked at this again and the method has now a string return type hint. So I changed this to check for an empty string instead. But:
getDefaultLayout()still has this return statementreturn \reset($plugins);and if$pluginsis an empty array, it will not return a string, but booleanfalse. This means that we could probably remove this check in TokenProvider entirely, since the Layout Manager will then generate a white page (wrong return type). Or we could fix the wrong return type. For now I didn't change anything about this in the layout manager, because unlike the other change in the layout manager that is a part of this patch, this bug fix is not needed to pass tests here, so it could be done in a follow-up issue, if desired.Comment #9
acbramley commentedIMO this functionality could live in a separate module. Judging by the activity on this issue, it's not something that seems to be needed by a lot of users and is a non-trivial amount of code and tests to include.
Comment #10
feyp commentedAlright, separate module is not gonna happen, so let's close this won't fix.