Several preprocess hook implementations in core themes define the `$variables` parameter without an explicit type declaration (e.g., `&$variables`).
Adding explicit `array` type hints improves developer experience and aligns with Drupal’s move toward stricter typing.
NA
Add `array` type hints to all preprocess hook method parameters where `$variables` (and similar parameters like `$form`, `$suggestions`, `$info`) are expected to be arrays.
Combine #3579925: Add missing type hint for $variables parameter in ClaroHooks.php
NA
NA
Method signatures for hook implementations are updated to include stricter parameter type declarations.
Show commandsStart 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:
compare changes, plain diff MR !15260
Comments
Comment #2
sivaji_ganesh_jojodae commentedComment #3
debdeep.mukhopadhyay commentedI am working on this.
Comment #4
nicxvan commentedThank you, this needs to be postponed I think on the remaining conversion.
Comment #5
sivaji_ganesh_jojodae commentedComment #6
sivaji_ganesh_jojodae commentedThank you for updating the issue appropriately. It would be helpful to link the blocking parent issue here for easier tracking and follow-up.
Comment #7
nicxvan commentedComment #8
sivaji_ganesh_jojodae commentedMoving to Active as #3579903: Eliminate Olivero and three test .theme files is fixed now.
Comment #9
sivaji_ganesh_jojodae commentedWorking on this.
Comment #10
nicxvan commentedComment #12
sivaji_ganesh_jojodae commentedComment #13
smustgrave commentedIs this something that we actually need to do? if so think it's so minor should expand scope to other themes.
Comment #14
smustgrave commentedLets least add #3579925: Add missing type hint for $variables parameter in ClaroHooks.php if we are doing this.
Comment #15
sivaji_ganesh_jojodae commentedIf the intention is to merge issues, the issue metadata (title, component, and summary) should be updated accordingly to reflect the combined scope. Inappropriate or inconsistent updates can make it harder for potential reviewers to understand the issue and may lead to it being overlooked or misinterpreted.
I would refrain from updates that do not align with the original scope or that disregard the interest of other participants in this issue.
Comment #16
smustgrave commentedYou are correct. I went ahead and did the title.
Again this is just adding arrays. Nothing complex so reviews are not complex. This really should of been a novice task for first time users but what's done is done. Lets combine.
Comment #17
smustgrave commentedIn case a new user wants to combine.
Comment #18
smustgrave commentedFixed the summary for you, removing the AI stuff.
Comment #19
smustgrave commentedPosted in #first-contribution if you don’t happen to get to it.
Thanks
Comment #20
sivaji_ganesh_jojodae commentedIt seems like the recent updates are moving toward a “process as proxy” approach.
Both issues already had the necessary PRs in place and were quite close to RTBC and being marked as fixed. Some of the recent metadata changes appear redundant and only moving the issue backward rather than helping it progress.
In particular, the updates in #3578084: Add return types to Twig implementations reverts change from my PR, which sounds rude and difficult to follow the direction of the issue and continue contributing effectively. It would be helpful to better understand how much autonomy contributors are expected to exercise in such cases.
For those continuing work on this issue, you can apply the changes from #3579925: Add missing type hint for $variables parameter in ClaroHooks.php using:
git cherry-pick 9ffc6e69a76f81241b00dadd3c82f89f1fafe709Comment #21
nicxvan commented@sivaji_ganesh_jojodae I understand it can sometimes be frustrating.
Determining the right scope in core can be complex.
The twig issue had a very targeted scope so adding more return types could be problematic if it had to be reverted so it's better to be tightly scoped.
This and the claro hooks are nearly identical and easy to review so they should be combined.
The good news is you can just apply the claro hook diff to this branch.
I would also look for the test modules and update the typing there too.
If you have further questions the #core-development channel in slack is a great place to discuss scoping issues.
We are always trying to balance the number of issues and the scope so it's not always easy to get correct when creating issues.
Comment #22
sivaji_ganesh_jojodae commented@nicxvan, thanks for the clarification, I appreciate the context. And thanks as well for pointing to the Slack channel — I’ll keep that in mind for discussing scope-related questions going forward.
Comment #23
sivaji_ganesh_jojodae commentedI've git cherry picked the changes from #3579925: Add missing type hint for $variables parameter in ClaroHooks.php and rebased.
Comment #25
smustgrave commentedThink this one is fine to go.
Comment #26
alexpottCommitted and pushed 387533dfcef to main and 8e3d0e2e6d8 to 11.x. Thanks!