Extensibility issues - Githubissues

php-gettext / Gettext

PHP library to collect and manipulate gettext (.po, .mo, .php, .json, etc)

MIT License

687 stars 134 forks source link

Extensibility issues #231

Closed schlessera closed 4 years ago

schlessera commented 4 years ago

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:

For non-final classes, default to static instead of self (late static binding) => this allows for all the code to properly support any extensions down the line.
For instantiations of classes, extract the actual classes either into a method/constant that can be overridden, or a factory

oscarotero commented 4 years ago

Yes, sure. Just try to do not include so many changes in the same PR, so it's easier to review :)

schlessera commented 4 years ago

I'll split it up into multiple PRs as needed then.

schlessera commented 4 years ago

@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.

schlessera commented 4 years ago

@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.

oscarotero commented 4 years ago

@schlessera The changes are right, so I just merge them and will be included in the next release.

schlessera commented 4 years ago

Awesome, thanks so much for the quick iteration!

oscarotero commented 4 years ago

FYI, I'm working in a new (rewritten) v5 version to solve some of the issues of this library:

Difficult to extend
It contains many extractors/generators that not all people need
Some spaguetti code

The idea is splitting this library in short packages that people can install if needed. The core package gettext/gettext only contains support for PO/MO files in addition to php arrays and code. The translator may be published as gettext/translator, same as other formats like gettext/xliff, gettext/twig, etc.

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.

rgpublic commented 4 years ago

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...

oscarotero commented 4 years ago

v4.8 released including this change.