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

BC break in #69 #70

Open
NiklasBr opened this issue Nov 18, 2024 · 6 comments · Fixed by #71
Open

BC break in #69 #70

NiklasBr opened this issue Nov 18, 2024 · 6 comments · Fixed by #71

Comments

@NiklasBr
Copy link

I can't comment there because you locked the pull request @mattamon, so I had to create this issue. Please add those changes to the 2.0 milestone or add a compatibility layer. Otherwise current installations will fails due to breaking backwards compatibility.

@jdreesen
Copy link
Contributor

I don't think thin bundle uses SemVer, as they do changes like this all the time ;)

@NiklasBr
Copy link
Author

It is part of Platform Version which does follow semver, so this could be added to 2025.1 but not 2024.4

https://github.com/pimcore/platform-version/blob/e65d9c4ad4b5a009dfc0086d3806bd7645ba8f40/composer.json#L38

@fashxp
Copy link
Member

fashxp commented Nov 18, 2024

This bundle should follow semver, yea.

But why does this PR break your code? Do you implement these interfaces for your own?

@NiklasBr
Copy link
Author

Yes, we use it. Were we not supposed to?

@mattamon
Copy link
Collaborator

@NiklasBr I am very sorry. This was a total oversight from our side.
This bundle was mainly created to be able to write Unit tests since with Static calls it is practically impossible.

We never ever imagined that someone would really implement those interfaces themselves.
I added a PR #71 with a caution notice.

Maybe a few words why we did not take care of semver here. With our platform-versions we can only do major versions once a year, thus only do BC-Breaks once a year. Since we heavily rely on this bundle in studio, and more and more functions arise, that are missing from this bundle, we wanted to implement them in a swift way so we can be on track with studio. If we do it otherwise, e.g. and we did not get all the dependencies for 2025.1, we would need to wait another year for studio to be released, since next BC-Breaks are only possible then with 2026.1. Since this is not an option we opted for the current course.

Again sorry if we caused you inconveniences.

What is our next goal? Since as we see there is demand for such things, can you please explain your uses case(s) why you decided to implement certain interfaces of this bundle. That would be really helpful for us to understand this better.
We will then discuss what to do here internally and come back to you.

@NiklasBr
Copy link
Author

You could have just added the method to the class implementing it in milestone 1.5.0 without changing the interface, and then done the interface change separately to the 2.0.0 milestone. Why did you not do that?

With our platform-versions we can only do major versions once a year, thus only do BC-Breaks once a year.

The Platform Version numbering scheme was not good, as this is not the first time you are confronted with the bad limitation you ("you" as in "Pimcore") imposed on yourself. I strongly suggest Pimcore improve the situation by dropping it soon in favour of normal versioning numbers not based on years like this. If instead you had followed a pattern like "Platform Version 1.0 to 1.x". "Platform 2.0 to 2.x" and so on this would not have been an issue. You could then have released a new Platform Version when it was ready instead of locking yourself into this problematic scheme. You could have released Platform Version 2.0 today and then kept 1.x alive for as long as you wanted.

We never ever imagined that someone would really implement those interfaces themselves.

Why not? Just like you, we have invested considerable effort overriding various Dao and Resolver classes to be able to run using Codeception without needing database calls or other service requests to instead serve proper mock data.

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 a pull request may close this issue.

4 participants