-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Extensibility issues #231
Comments
Yes, sure. |
I'll split it up into multiple PRs as needed then. |
The FunctionsScanner and StringReader classes are directly referenced in the pieces of logic that use them. If you want to extend one of these, you have to duplicate a big chunk of logic just to change the class reference for the `new` call. This PR extracts the class name into a protected static property that can be changed in extending classes, to avoid the need to copy-paste big chunks of logic when they need to be changed. Related issue: php-gettext#231
@oscarotero I have created 4 separate PRs with the changes I think are needed to make the classes properly extensible while still keeping everything pragmatic. I have added lots of reasoning in commit messages to explain each of the changes, and split it up into separate commits so they can be cherry-picked if needed. Looking forward to reading your feedback. |
@oscarotero I saw you already merged 2 of the 4 PRs. Are there any short-term plans of including these in a next tagged release? I'm not asking to put pressure on you, just want to find out whether it makes sense to work on a work-around for our code in the mean-time, or just wait it out until the tagged release is there. |
@schlessera The changes are right, so I just merge them and will be included in the next release. |
Awesome, thanks so much for the quick iteration! |
FYI, I'm working in a new (rewritten) v5 version to solve some of the issues of this library:
The idea is splitting this library in short packages that people can install if needed. The core package If you want to take a look to provide feedback, you can do here: https://github.com/oscarotero/Gettext/tree/v5-dev This is a work in progress yet. |
As a quick side note, I'd like to add that I find it currently very hard to extend the PHP function argument scanning. I'm currently trying to have it parse Drupal's t()-Method which has the form: t("translatable string",[],["context"=>"Yada yada"]). The information in that array is currently not parsed at all because it just looks for T_STRING. I've copied the whole code in getFunctions() and adapted it to my needs but this is of course very ugly :-(. Would be far better if somehow one could only extend the parsing of a single function argument without rewriting the whole function... |
v4.8 released including this change. |
We're using this library in
wp-cli/i18n-command
as the new basis to improve the translations build step for WordPress Core.To extend it so that it does what we need, we need to jump through some hoops in various places of the code, because extensibility is made difficult by a few practices like ignoring late static binding or directly instantiating specific classes within a big chunk of logic.
Would you be open to a pull request that (in a generally backwards compatible way) improves extensibility across the board?
This would allow us to make the changes we need without copy-pasting big chunks of code and needing to keep them in sync.
The main changes I would like to introduce are:
static
instead ofself
(late static binding) => this allows for all the code to properly support any extensions down the line.The text was updated successfully, but these errors were encountered: