Skip to content

Add return type annotations on Twig 3 to prepare Twig 4#4534

Merged
fabpot merged 1 commit intotwigphp:3.xfrom
smnandre:add-return-types-before-twig4
Jan 24, 2025
Merged

Add return type annotations on Twig 3 to prepare Twig 4#4534
fabpot merged 1 commit intotwigphp:3.xfrom
smnandre:add-return-types-before-twig4

Conversation

@smnandre
Copy link
Contributor

@smnandre smnandre commented Jan 5, 2025

Started to add missing type annotation following @stof comment here.

Update: I checked here every class (but one) in the src directory.

But you should probably certainely have another look.

Remains:

  • src/ExpressionParser.php, where i'm not confident enough about the best return types (AbstractExpression|null works almost everytime, but lack some precision)
  • extra directory
  • tests directory

I'm happy to let them to someone else.... this was less fun than I expected 😅

@smnandre
Copy link
Contributor Author

smnandre commented Jan 5, 2025

(fabbot unrelated)

@smnandre
Copy link
Contributor Author

smnandre commented Jan 7, 2025

(fixed my mistakes, all seems ok now)

}

private function needEscaping()
private function needEscaping(): string|bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string|false actually. true is not a valid strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"true" will wait Twig4 / PHP 8.2 no ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean here. true would indeed require PHP 8.2 to be used. But the one we need here is false, not true

@smnandre
Copy link
Contributor Author

smnandre commented Jan 8, 2025

Thank you very much @stof! (and sorry i make that many mistakes 😮‍💨 )

I've fixed all your comments

@smnandre smnandre requested a review from stof January 10, 2025 01:06
@fabpot fabpot force-pushed the add-return-types-before-twig4 branch from de4dc5e to 82be134 Compare January 24, 2025 10:33
@fabpot
Copy link
Contributor

fabpot commented Jan 24, 2025

Thank you @smnandre.

@fabpot fabpot merged commit 72c91f6 into twigphp:3.x Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants