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

Command icon 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:

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Not sure how to best review this but return types are added and baseline/ignored file is smaller. Pipeline is green

godotislate’s picture

Status: Reviewed & tested by the community » Needs work

There are a couple open suggestions on the MR.

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

sivaji_ganesh_jojodae’s picture

Status: Needs work » Needs review

I've addressed the suggestion on the MR.

longwave’s picture

Title: Add return types to Twig implementations » Add argument and return types to Twig implementations

The 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.

smustgrave’s picture

Status: Needs review » Needs work

Think 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

sivaji_ganesh_jojodae’s picture

Status: Needs work » Needs review

@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.

smustgrave’s picture

Status: Needs review » Needs work

@sivaji can you please revert your changes from before. Not the rebase the suggestions you applied please

sivaji_ganesh_jojodae’s picture

Status: Needs work » Needs review

I 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.

smustgrave’s picture

No worries at all, its a learning process. Thank you for reverting that.

longwave’s picture

Title: Add argument and return types to Twig implementations » Add return types to Twig implementations
sivaji_ganesh_jojodae’s picture

smustgrave’s picture

I’m going to request we don’t open very specific follow ups like that. They’ll have to be well scoped and thought out.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

  • amateescu committed 5f59e9f2 on main
    task: #3578084 Add return types to Twig implementations
    
    By: longwave
    By...
amateescu’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5f59e9f and pushed to main. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.