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

Fix named parameter used for trans filter #588

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

Steveb-p
Copy link

@Steveb-p Steveb-p commented Aug 21, 2024

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes/no
Fixed tickets
License Apache2

Description

This PR allows TwigExtractor to recognize a following case:

{{ 'Some text that belongs to another translation domain'|trans(domain='not_messages_domain') }}

I'd love to have this in 1.x series (since I still have projects that depend on them), but I don't know if it's still under maintenance.

Could you point me to relevant tests to add a case for them?

Todos

  • Tests
  • Documentation
  • Changelog

@goetas
Copy link
Collaborator

goetas commented Aug 21, 2024

Hi, thanks for your contribution. Could you please fix the failing tests?

@Steveb-p
Copy link
Author

@goetas I've updated the CI to use nikic/php-parser ^4.9. simple-phpunit installed v5 without additional instructions, causing PHP fatal error.

See symfony/symfony#53459 (comment) as the source of the workaround.

@Steveb-p
Copy link
Author

Steveb-p commented Aug 22, 2024

@goetas I've run tests on my fork (https://github.com/Steveb-p/JMSTranslationBundle/actions/runs/10504385228) to ensure they will actually pass on GitHub actions.

I've moved those changes to #589 so they can be viewed separately. Once that is confirmed, I'll drop those commits from here.

@goetas goetas closed this Aug 22, 2024
@goetas goetas reopened this Aug 22, 2024
@goetas goetas merged commit d14af48 into schmittjoh:1.x Aug 22, 2024
13 checks passed
@goetas
Copy link
Collaborator

goetas commented Aug 22, 2024

Thanks

@Steveb-p
Copy link
Author

Are you okay with merging this (and resolving any conflicts) into master, or should I provide a PR for you? :)

@goetas
Copy link
Collaborator

goetas commented Aug 22, 2024

It would be great if you could do it

@goetas
Copy link
Collaborator

goetas commented Aug 22, 2024

🙏

@goetas
Copy link
Collaborator

goetas commented Aug 22, 2024

I guess that you need a new tag right?

@Steveb-p
Copy link
Author

Yeah, I'd be grateful for a release in the near future :) but no rush, whenever you have some spare time 🙏 🙇

@Steveb-p
Copy link
Author

Merge is available in #590.

phpunit.xml changes are "ignored", as they are not needed on the master branch.

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.

2 participants