Skip to content

Conversation

@come-nc
Copy link
Collaborator

@come-nc come-nc commented Mar 6, 2025

Fix #34

Not sure about all the changes, I had to pull OCP so that rector finds the classes.
I had to delete my composer.lock and start from scratch, and I set platform php to 8.1 to avoid composer pulling a psalm too recent (otherwise it only runs on latest PHP).

@come-nc come-nc added the enhancement New feature or request label Mar 6, 2025
@come-nc come-nc self-assigned this Mar 6, 2025
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Copy link
Collaborator Author

come-nc commented Mar 6, 2025

The lowest tests fails because I did not set a minimum version for nextcloud/ocp I suppose. Maybe we can set that to 25 as that’s the oldest set we have?

We have to require a version of nextcloud/ocp that contains the classes we
 use in the tested sets. Otherwise tests for 26 and 27 sets will fail
 when pulling the lowest version of dependendies.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the enh/update-rector-to-2.0 branch from 1c31a26 to bbf5de8 Compare March 10, 2025 09:10
@come-nc
Copy link
Collaborator Author

come-nc commented Mar 10, 2025

The lowest tests fails because I did not set a minimum version for nextcloud/ocp I suppose. Maybe we can set that to 25 as that’s the oldest set we have?

I had to require 27 so that tests on 26 and 27 sets pass.

@provokateurin
Copy link
Contributor

So it has to be the highest supported version so all changes can be made?

@come-nc
Copy link
Collaborator Author

come-nc commented Mar 10, 2025

So it has to be the highest supported version so all changes can be made?

Yeah, seems like it.
In applications it may be annoying, because you usually require nextcloud/ocp for the lowest supported version, not the highest.
Solutions:

  1. Version this library in sync with nextcloud/ocp, release a version for each NC major
  2. Change the tests to require the nextcloud/ocp of a version before running the associated set test suite

For now maybe we can merge as-is, as 27 is not that recent and should be ok as a requirement for apps using nextcloud/rector?

But yeah later when we populate sets for 31+ it’s gonna be a problem, so maybe we tackle it now 🤷

@provokateurin
Copy link
Contributor

Sounds good to me, although we can and should prevent the ocp version conflict in apps by putting rector into vendor-bin.

@come-nc come-nc merged commit 2b1b6d1 into main Mar 10, 2025
22 checks passed
@come-nc come-nc deleted the enh/update-rector-to-2.0 branch March 10, 2025 13:22
@provokateurin
Copy link
Contributor

New release? @ChristophWurst

@come-nc come-nc changed the title Enh/update rector to 2.0 Update rector to 2.0 Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support rector ^2.0

4 participants