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

PSR-16 standard compliance #120

Draft
wants to merge 7 commits into
base: 2.27.x
Choose a base branch
from
Draft

Conversation

PowerKiKi
Copy link
Contributor

Closes #49

Q A
Documentation no
Bugfix no
BC Break yes
New Feature yes
RFC no
QA no

Description

This is a revival of #50, to bring support for PSR-16, and with the goal to release as a major version.

I think the decorator pattern suggested in #50 is a bit too cumbersome for a feature that is supposed to be used very often. Keeping things well-integrated in the Translator class keep it simple to use, and dropping support for CacheStorage makes maintenance easier. Both of those decisions seems to be the best for the long term, even though it requires a major version.

cedric-anne and others added 5 commits April 25, 2024 14:51
Closes laminas#49

Signed-off-by: Cédric Anne <cedric.anne@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
@PowerKiKi PowerKiKi marked this pull request as ready for review April 25, 2024 08:27
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>
@froschdesign
Copy link
Member

@PowerKiKi

I think the decorator pattern suggested in #50 is a bit too cumbersome for a feature that is supposed to be used very often.

To be clear, the suggestion for the decorator is the fact that a support for PSR-16 can be released with a minor version. This means faster availability.
A new major version will not be created just for this one change. See the other open pull requests and issue reports.

@froschdesign froschdesign added this to the 3.0.0 milestone Apr 25, 2024
@froschdesign froschdesign self-assigned this Apr 25, 2024
@froschdesign froschdesign marked this pull request as draft April 25, 2024 11:57
@PowerKiKi
Copy link
Contributor Author

new major version will not be created just for this one change

Fair enough, I am in no hurry. Especially because in the meantime the decorator technique can be implemented in projects that require PSR-16 support.

Would you mind explaining the reasoning behind putting this PR back to draft status ? IMHO this is ready for review and, if the review is ok, should be merged into whatever branch will end up as the next major.

@froschdesign
Copy link
Member

@PowerKiKi

Fair enough, I am in no hurry.

👍🏻

Would you mind explaining the reasoning behind putting this PR back to draft status ?

I simply want to avoid merging this pull request too early.
You have correctly noticed that it is the wrong branch. And in addition, the entire Translator class is too bloated, it simply does too much instead of just translating.

The interface defines only to methods:

interface TranslatorInterface
{
/**
* Translate a message.
*
* @param string $message
* @param string $textDomain
* @param string $locale
* @return string
*/
public function translate($message, $textDomain = 'default', $locale = null);
/**
* Translate a plural message.
*
* @param string $singular
* @param string $plural
* @param int $number
* @param string $textDomain
* @param string|null $locale
* @return string
*/
public function translatePlural(
$singular,
$plural,
$number,
$textDomain = 'default',
$locale = null
);
}

But the Translator class has so many more public methods and does too much:

Therefore, a fundamental redesign must be considered for the next major version. This does not mean that the features cache, the events or the factory should be removed, but whether they all belong in this one class. And if it would not be better to focus only on the essential in this class and in this way considerably extend the extension / reuse.

@froschdesign
Copy link
Member

@PowerKiKi
By the way: Thank you for pushing this topic! 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: PSR-16 (cache) compatibility
3 participants