Problem/Motivation
In 11.2 we tightened up some of our deprecation skips: #3523383: Tidy up and tighten deprecation skips
%Method "Twig\\Extension\\ExtensionInterface::[^"]+" might add "array" as a native return type declaration in the future. Do the same in implementation "Drupal\\Core\\Template\\TwigExtension" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\Loader\\FilesystemLoader::findTemplate\(\)" might add "\?string" as a native return type declaration in the future. Do the same in child class "Drupal\\Core\\Template\\Loader\\FilesystemLoader" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\Loader\\LoaderInterface::exists\(\)" might add "bool" as a native return type declaration in the future. Do the same in implementation "Drupal\\Core\\Template\\Loader\\StringLoader" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\Node\\Node::compile\(\)" might add "void" as a native return type declaration in the future. Do the same in child class "Drupal\\Core\\Template\\TwigNodeCheckDeprecations" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\Node\\Node::compile\(\)" might add "void" as a native return type declaration in the future. Do the same in child class "Drupal\\Core\\Template\\TwigNodeTrans" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\NodeVisitor\\NodeVisitorInterface::[^"]+" might add "[^"]+" as a native return type declaration in the future. Do the same in implementation "Drupal\\Core\\Template\\RemoveCheckToStringNodeVisitor" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\NodeVisitor\\NodeVisitorInterface::[^"]+" might add "[^"]+" as a native return type declaration in the future. Do the same in implementation "Drupal\\Core\\Template\\TwigNodeVisitor" now to avoid errors or add an explicit @return annotation to suppress this message%
%Method "Twig\\TokenParser\\TokenParserInterface::[^"]+" might add "[^"]+" as a native return type declaration in the future. Do the same in implementation "Drupal\\Core\\Template\\TwigTransTokenParser" now to avoid errors or add an explicit @return annotation to suppress this message%
In 12.0 we can remove these skips and add the return types to avoid issues if/when Twig 4 is released.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3578084
Show commands
Start 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:
- 3578084-add-return-types
changes, plain diff MR !15034
Comments
Comment #3
longwaveComment #4
smustgrave commentedNot sure how to best review this but return types are added and baseline/ignored file is smaller. Pipeline is green
Comment #5
godotislateThere are a couple open suggestions on the MR.
Comment #7
sivaji_ganesh_jojodae commentedI've addressed the suggestion on the MR.
Comment #8
longwaveThe issue was only to add return types to solve the deprecation notices, so there is some scope creep here. In general we try to fix only one thing at a time in an issue.
We also don't allow additional docs alongside @inheritdoc, so I removed that change.
Comment #9
smustgrave commentedThink some of the changes need to be reverted. Since I reverted to Reporter to rerun pipelines for others I can’t close threads but I should have put checkmarks they weren’t needed
Comment #10
sivaji_ganesh_jojodae commented@longwave, thanks for the clarification, and apologies for the earlier scope creep. I had initially included argument type hints along with return types.
I appreciate you updating the issue scope to accommodate both. I’ve now rebased the MR and rerun the pipeline; everything looks good from my side and should be ready for review.
Please let me know if anything else needs to be adjusted.
Comment #11
smustgrave commented@sivaji can you please revert your changes from before. Not the rebase the suggestions you applied please
Comment #12
sivaji_ganesh_jojodae commentedI had interpreted the title update in #8 as including argument types as well—apologies for the confusion. I’ve now reverted those changes and kept only the return types.
Comment #13
smustgrave commentedNo worries at all, its a learning process. Thank you for reverting that.
Comment #14
longwaveComment #15
sivaji_ganesh_jojodae commentedI've added #3582567: Add missing parameter type hints to Twig loader implementations to follow up on the type hint.
Comment #16
smustgrave commentedI’m going to request we don’t open very specific follow ups like that. They’ll have to be well scoped and thought out.
Comment #17
dcam commentedI don't have anything to add. I double-checked all of the return types with the Twig interfaces in GitHub, mostly because of the deprecation notices where it wasn't specific about the affected methods or types. They're all accurate. The MR looks good to me.
Comment #19
amateescu commentedCommitted 5f59e9f and pushed to main. Thanks!