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

[2.x] Add namespaced aliases #2486

Merged
merged 1 commit into from May 25, 2017
Merged

[2.x] Add namespaced aliases #2486

merged 1 commit into from May 25, 2017

Conversation

nicolas-grekas
Copy link
Contributor

The 2.x-only part of #2484

@fabpot
Copy link
Contributor

fabpot commented May 25, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 71265c4 into twigphp:2.x May 25, 2017
fabpot added a commit that referenced this pull request May 25, 2017
This PR was merged into the 2.x branch.

Discussion
----------

[2.x] Add namespaced aliases

The 2.x-only part of #2484

Commits
-------

71265c4 [2.x] Add namespaced aliases
@nicolas-grekas nicolas-grekas deleted the ns2 branch May 30, 2017 09:31
@robfrawley
Copy link
Contributor

@nicolas-grekas After this change was merged, when running our test suite for the liip/imagine-bundle package using twig@2.x-dev, a PHP error is being thrown with the following message when defining a custom extension that returns a single Twig filter (nothing special about it).

Cannot declare class Twig\TwigFilter, because the name is already in use

We are referencing TwigFilter using the Twig_SimpleFilter class name, but I can also confirm that when using the new namespaced class name Twig\TwigFilter the error is still thrown. Downgrading to 1.x-dev fixes the issue so it appears only the 2.x branch contains this problem.

Removing the following line from lib/Twig/SimpleFilter.php fixes this issue.

class_alias('Twig_SimpleFilter', 'Twig\TwigFilter', false);

But it I'm not sure it's that simple. Based on a cursory examination, it appears like Twig/SimpleFilter.php:23 and Twig/Filter.php:142 are the cause of the error, because one includes the other and they both declare an alias for Twig\TwigFilter. Not positive on how to overcome this, but it doesn't appear they can both alias the same class without a class exists check or another mechanism in place.

If you'd like me to open a proper issue instead of bringing this up on a closed PR, let me know and I'll make it happen.

@robfrawley
Copy link
Contributor

Also, it seems odd that the test suite for this bundle didn't fail; shouldn't there be unit tests somewhere instantiating a filter to test it can be instantiated?

@nicolas-grekas
Copy link
Contributor Author

Please don't comment on closed PR, it makes tracking harder for everyone.
Can you please check #2495 a report there, or open an issue here if more sensible to you?

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.

None yet

3 participants