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

Removes deprecated class Piwik\Translate #15586

Merged
merged 6 commits into from Feb 20, 2020
Merged

Removes deprecated class Piwik\Translate #15586

merged 6 commits into from Feb 20, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 17, 2020

refs #8567

@sgiehl sgiehl added the Needs Review PRs that need a code review label Feb 17, 2020
@sgiehl sgiehl marked this pull request as ready for review February 17, 2020 15:51
@tsteur
Copy link
Member

tsteur commented Feb 17, 2020

BTW: I'm wondering in that case it it's worth removing the class? Not sure there is much of a down side to keep it and at the same time it might force heaps of plugins to change the code maybe? Might be the same for some other classes /methods. Wondering if sometimes we could just keep the old classes for simplicity of BC. In this case it wouldn't hurt us much maybe to keep that old class?

Of course we'd still ideally change some usages to be a good example and make use of it correctly ideally.

In general since we manage > 50 plugins it might be easier to sometimes just keep the old classes if they are as simple as the translate class

CHANGELOG.md Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Feb 17, 2020

We already deprecated that class in Piwik 2.11 and kept it in 3.x for BC. At some point we need to break BC and remove such stuff otherwise we will mess up the code with more and more such stuff 🤷‍♂️

@tsteur
Copy link
Member

tsteur commented Feb 17, 2020

At some point we need to break BC and remove such stuff otherwise we will mess up the code with more and more such stuff 🤷‍♂

Not sure how one file would mess up the code there? Of course it always depends but in this case not seeing it so much. There doesn't seem to be much of a harm as it's only forwarding methods anyway. If it helps keeping plugins easily compatible and less pain for us and developers be great. Personally, I love eg how Android and iOS etc keep BC basically almost forever (not saying we should always do this but when it's easy like there might be worth it)

@sgiehl
Copy link
Member Author

sgiehl commented Feb 17, 2020 via email

@tsteur
Copy link
Member

tsteur commented Feb 17, 2020

Not sure. I reckon in that case there might not even be a need to remove it in Matomo 5 either. I'm thinking if we can keep BC whenever it's easy for us we probably should. Could even remove the deprecated flag there. In development mode we could trigger a notice though maybe and recommend using a different way. 👍

@sgiehl
Copy link
Member Author

sgiehl commented Feb 18, 2020

@tsteur readded the file, triggering deprecation notices in development mode

@tsteur
Copy link
Member

tsteur commented Feb 18, 2020

👍

core/Translate.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Feb 19, 2020

@tsteur let me know if you think it's ready to merge. Would do that in all linked repos then and update the submodules before merge.

@tsteur
Copy link
Member

tsteur commented Feb 19, 2020

@sgiehl I haven't really looked (only very quickly) but reckon it's fine 👍 Unfortunately won't be able to do any reviews in the next 7 days :(

@sgiehl sgiehl merged commit fa23dc7 into 4.x-dev Feb 20, 2020
@sgiehl sgiehl deleted the removetranslate branch February 20, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants