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

118 - Drop support of older php versions #132

Merged
merged 9 commits into from May 17, 2024

Conversation

lutdev
Copy link
Contributor

@lutdev lutdev commented May 1, 2024

Description:

PR for #118

@michalkleiner
Copy link
Contributor

Hi @lutdev. Just a quick question here — the issue seemed more like a question (are we going to remove legacy PHP support?) rather than an issue or bug. I believe this module can work with PHP versions from say 7.0 to 8.3, so why wouldn't we allow that? It doesn't use modern constructs that wouldn't work on say 7.0, so we may as well just keep the support until we have to drop it for some reason.

@sgiehl
Copy link
Member

sgiehl commented May 2, 2024

I agree with @michalkleiner. The php tracker here at least needs to support the PHP versions Matomo itself supports. Currently that would be PHP 7.2+

@lutdev
Copy link
Contributor Author

lutdev commented May 2, 2024

@michalkleiner yes, it was a question, for sure. I decided to drop the support 7.0 version because with it we can't use features that 8.1+ provides for us.

It doesn't use modern constructs that wouldn't work on say 7.0, so we may as well just keep the support until we have to drop it for some reason.

I don't want to do a lot of breaking changes in one PR :) That's why I didn't use features from 8.1+

Thanx @sgiehl for the information. It's a very strong argument for me.

I suggest to change the minimal version to 7.2. What do you think about it?

@michalkleiner
Copy link
Contributor

@michalkleiner yes, it was a question, for sure. I decided to drop the support 7.0 version because with it we can't use features that 8.1+ provides for us.

It doesn't use modern constructs that wouldn't work on say 7.0, so we may as well just keep the support until we have to drop it for some reason.

I don't want to do a lot of breaking changes in one PR :) That's why I didn't use features from 8.1+

In that case we can increase the minimum required version in the same PR where we refactor to use modern 8.1+ features. Until then it is not needed to remove the 7.2 support.

I suggest to change the minimal version to 7.2. What do you think about it?

7.2 minimum is fine with me.

@lutdev
Copy link
Contributor Author

lutdev commented May 6, 2024

@michalkleiner perfect!) What are the next steps? What should I do?
Would be great if we update CHANGELOG.md, but what version should we mention?

@michalkleiner
Copy link
Contributor

@michalkleiner perfect!) What are the next steps? What should I do? Would be great if we update CHANGELOG.md, but what version should we mention?

Let's reinstate the constant that you removed even if it's not directly used here, it would be a BC breaking change.

You can also update changelog and I think the changes here should target 3.3.0.

@lutdev
Copy link
Contributor Author

lutdev commented May 8, 2024

@michalkleiner thank you!) I updated CHANGELOG.md

CHANGELOG.md Outdated Show resolved Hide resolved
@michalkleiner
Copy link
Contributor

@sgiehl this is now fine with me. Is it ok to merge this and tag 3.3.0?

@lutdev
Copy link
Contributor Author

lutdev commented May 13, 2024

@michalkleiner I updated CHANGELOG.MD and added information about dynamic properties

@lutdev
Copy link
Contributor Author

lutdev commented May 15, 2024

@sgiehl just a friendly reminder about PR :)

@sgiehl sgiehl merged commit 6dd138e into matomo-org:master May 17, 2024
@lutdev lutdev deleted the 118-php-version branch May 21, 2024 08:20
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.

None yet

3 participants