Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force loading class aliases used in typehints #2887

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
10 participants
@stof
Copy link
Contributor

stof commented Mar 12, 2019

Fixes #2886

@stof

This comment has been minimized.

Copy link
Contributor Author

stof commented Mar 12, 2019

I haven't added tests for this, because it is hard to reproduce in a testsuite. The error depends on the order in which classes are loaded. If the typehint class is already loaded (and so its alias is already defined), the error does not happen (which is exactly how this gets solved).

@@ -13,6 +13,9 @@
use Twig\Environment;
// Ensure that the aliased name is loaded to keep BC for classes implementing the typehint with the old aliased name.
class_exists('Twig\Environment');

This comment has been minimized.

@nicwortel

nicwortel Mar 12, 2019

Is it enough to add this to the NodeVisitorInterface, or should it also be added to implementations such as AbstractNodeVisitor? I'm asking because I found this PR because we were getting:

Compile Error: Declaration of Translation\Bundle\Twig\Visitor\NormalizingNodeVisitor::doEnterNode(Twig_Node $node, Twig_Environment $env) must be compatible with Twig\NodeVisitor\AbstractNodeVisitor::doEnterNode(Twig\Node\Node $node, Twig\Environment $env)

Will the fact that the AbstractNodeVisitor implements the NodeVisitorInterface be enough to trigger the autoloading?

This comment has been minimized.

@Kocal

Kocal Mar 12, 2019

Contributor

Also see php-translation/symfony-bundle#288, we have the same issue there

This comment has been minimized.

@nicwortel

nicwortel Mar 12, 2019

@Kocal that is indeed the bundle we are using, too 😄 I probably should have included the upstreamdownstream issue / repository

This comment has been minimized.

@stof

stof Mar 12, 2019

Author Contributor

@nicwortel yes, it will be enough, as AbstractNodeVisitor implementing the interface will force the interface to be loaded.

@kiler129

This comment has been minimized.

Copy link

kiler129 commented Mar 12, 2019

To avoid problems after this path is merged library authors which rely on Twig and use non-NS class references basically need to include:

    "conflict": {
        "twig/twig": "2.7.0 || 1.38.0"
    },

This will prevent installing 2.7.0 and breaking builds :D

Edit: add 1.38.0 conflict

@stof

This comment has been minimized.

Copy link
Contributor Author

stof commented Mar 12, 2019

note that 1.38 is also affected.

@wadjeroudi

This comment has been minimized.

Copy link

wadjeroudi commented Mar 12, 2019

@kiler129 The 2.7.1 release should be the good one, why exclude it ?

@Gummibeer

This comment has been minimized.

Copy link

Gummibeer commented Mar 12, 2019

@kiler129 same for 1.38.0 :/

@kiler129

This comment has been minimized.

Copy link

kiler129 commented Mar 12, 2019

@wadjeroudi @Gummibeer misread the changelog - I was under impression 2.7.0 introduced the break while 2.7.1 was a security fix while both arrived in 2.7.0. Corrected the comment.

Gummibeer added a commit to Gummibeer/assetic that referenced this pull request Mar 12, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Mar 12, 2019

Thank you @stof.

@fabpot fabpot merged commit 65c0559 into twigphp:1.x Mar 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Mar 12, 2019

bug #2887 Force loading class aliases used in typehints (stof)
This PR was merged into the 1.x branch.

Discussion
----------

Force loading class aliases used in typehints

Fixes #2886

Commits
-------

65c0559 force loading class aliases used in typehints
@kiler129

This comment has been minimized.

Copy link

kiler129 commented Mar 12, 2019

@fabpot Can you maybe tag this as 2.7.1?

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Mar 12, 2019

@kiler129 Working as fast as possible. I asked people to test the code before release, tweeted about it, but apparently nobody cared back then.

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Mar 12, 2019

1.38.1 and 2.7.1 released.

@stof stof deleted the stof:fix_aliasing branch Mar 12, 2019

@kiler129

This comment has been minimized.

Copy link

kiler129 commented Mar 12, 2019

@fabpot @stof Thank you! You're doing an amazing job for open source :)

@goba

This comment has been minimized.

Copy link

goba commented Mar 12, 2019

Thanks @stof and @fabpot!

@Natshah

This comment has been minimized.

Copy link

Natshah commented Mar 12, 2019

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.