Skip to content

chore(twig): replace class aliases by class namespaces #290

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

Merged

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Mar 13, 2019

It's a following of #288 (comment), since Twig 3 will remove non-namespaced classes.


EDIT: hum, there is a lot of Travis builds that fail:

  • either because Composer ran out of memory (see chore(travis): disable Composer's memory limit #291)
  • either because deprecations make PHPUnit exit with status code different of 0,
  • either because they are failing tests with new_c and old_d (which appears on other PRs too) :(

@hvt
Copy link

hvt commented Apr 3, 2019

Would be great if this could get merged. Twig namespaces are already introduced in Twig 1.x, so I do not believe any problems would come with these changes.

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Thank you for doing this "chore" @Kocal and sorry for the delay. Unfortunately, probably only @Nyholm can merge PRs with failed TravisCI builds, so we need to wait for him here too. Thank you all for your patience.

These changes look good and safe to me 👍

@Kocal
Copy link
Contributor Author

Kocal commented Apr 15, 2019

Don't worry, we don't have those issues anymore since twigphp/Twig#2887 was quickly merged and released after twigphp/Twig#2863. 👍

@bocharsky-bw
Copy link
Member

Hm, I think we need to double check in what version those namespaces were introduced and bump Twig minimum constraint in composer.json to it. Wdyt @Kocal ? Because right now I think we may have valid broken tests, let's see if tests pass

@Kocal
Copy link
Contributor Author

Kocal commented Jun 25, 2019

It makes sens, namespace classes as default classes have been introduced in Twig 2.7.0 and Twig 1.38.0.

But I don't know how I should modifiy the twig/twig constraint, I think there is a reason for using <1.39 || ^2.0,<2.8 but I don't want to break anything 😄

@bocharsky-bw
Copy link
Member

Thank you for providing more information about it! I bumped Twig version in 8cf4938 - now tests should pass. I think I'm fine with using namespaces but would like to hear more opinions on it.

@Nyholm
Copy link
Member

Nyholm commented Jul 19, 2019

Oh, This PR looks great. I will rebase it and merge it when tests are green.
Good job!

@Nyholm Nyholm force-pushed the chore/twig-from-aliases-to-namespaces branch from b508c40 to 20c41c0 Compare July 20, 2019 10:52
@Nyholm Nyholm merged commit a485b2e into php-translation:master Jul 20, 2019
@Kocal Kocal deleted the chore/twig-from-aliases-to-namespaces branch July 20, 2019 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants