Fix getTranslationNodeVisitor() return type #37687
Conversation
fabpot
left a comment
fabpot
left a comment
There was a problem hiding this comment.
This change should be for master.
| private $translationNodeVisitor; | ||
|
|
||
| public function __construct(TranslatorInterface $translator = null, NodeVisitorInterface $translationNodeVisitor = null) | ||
| public function __construct(TranslatorInterface $translator = null, TranslationNodeVisitorInterface $translationNodeVisitor = null) |
There was a problem hiding this comment.
Let's not create a new interface and type hint with TranslationNodeVisitor instead.
There was a problem hiding this comment.
That would limit flexibility without much benefit.
I have a use case for passing a different node visitor into this class (basically overwriting the trans function and adding an extra parameter). That was possible in symfony 4.4, because there was no return typehint on the getter. I came across this trying to upgrade to symfony ^5.0. Typehinting on the concrete class here would make that impossible, since the concrete class is final.
It's up to you. If you still prefer the concrete class typehint then I will change it. Let me know @fabpot 👍
|
Not a big fan of adding such an interface TBH. Your implementation extends the core one, right? In which case, type hinting with the class is not an issue. |
|
My implementation does not extend the core one as the core one is declared final. We simply overwrite the TranslationExtension service using the core class and only passing a different NodeVisitor as second argument As it stands we've already had to copy and modify a few core classes (TransNode for example) to make our implementation work, so to upgrade to symfony 5 we could do the same with TranslationExtension. I will change the typehint to |
|
Looks like there is an unrelated commit in the PR. Can you get rid of it? |
Done @fabpot 👍 |
|
Thank you @dbrekelmans. |
4 participants
When constructing the
TranslationExtensionwith anyNodeVisitorInterfaceother thanTranslationNodeVisitor, you will get a type error when callinggetTranslationNodeVisitor(). This PR fixes that by extracting a newTranslationNodeVisitorInterface.