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

Fixed ZF1F version in composer.json to avoid conflicts with our patches #3475

Merged
merged 3 commits into from
Aug 31, 2023
Merged

Fixed ZF1F version in composer.json to avoid conflicts with our patches #3475

merged 3 commits into from
Aug 31, 2023

Conversation

fballiano
Copy link
Contributor

when implementing our composer patches for ZF1F we didn't consider that our 20.1.0 tagged release can't now run a composer install because ZF1F is now been updated and the patches can't be applied anymore.

Explanation at #3474

Thus we have to specify the exact version number of ZF1F in the composer.json.

Fixes #3474

@github-actions github-actions bot added the composer Relates to composer.json label Aug 29, 2023
ADDISON74
ADDISON74 previously approved these changes Aug 29, 2023
@fballiano
Copy link
Contributor Author

composer validation is complaining but we have to do it this way... I don't see any other possibility

@ma4nn
Copy link
Contributor

ma4nn commented Aug 30, 2023

composer validation is complaining but we have to do it this way... I don't see any other possibility

as mentioned in #3474 another possibility would be to use composer patches plugins such as vaimo/composer-patches that can handle version constraints for the patches

@fballiano
Copy link
Contributor Author

@ma4nn so sorry, I'll check it out immediately!

@fballiano
Copy link
Contributor Author

fballiano commented Aug 30, 2023

so, I was checking this matter more in detail, but I still think we should have a strict version dependency here.

let's make this example

  • now main is compatible with latest version of zf1f
  • we can't add any version constraint to the patches because we have no idea if some of them will ever be integrated in zf1f in the future
  • 1.23.5 could have one of the patches, or not, so we can't constraint "now" to "use this patch only if <=1.23.5 because we may need the patch in the next version, or not

am I getting something wrong?

@fballiano
Copy link
Contributor Author

@kiatng @elidrissidev @colinmollenhour @ADDISON74 we've to address this as soon as possible and tag a new release, at this moment all installations via composer are broken

@ma4nn
Copy link
Contributor

ma4nn commented Aug 30, 2023

@fballiano yes you're right, there is no optimal solution.

My (untested) idea was to constraint e.g. the MAG-1.9.3.9.patch to "shardj/zf1-future": "< 1.22.1".

Imho the whole idea of the composer patches is to live with the fact that there may be future updates to the patched library that will make your patch fail as you mentioned. In this case one could either add a patches-ignore section to their own composer.json or wait until the patch version constraint is updated with the next OpenMage version.

This approach has the benefit that you profit by updates of the zf1-future library without releasing a new OpenMage version which is very important imho.

@fballiano
Copy link
Contributor Author

My (untested) idea was to constraint e.g. the MAG-1.9.3.9.patch to "shardj/zf1-future": "< 1.22.1".

sure, but that patch is already removed from our composer.json for, for a next release, we don't have a need for the version contstraint, but it doesn't fix the problem anyway, sadly

This approach has the benefit that you profit by updates of the zf1-future library without releasing a new OpenMage version which is very important imho.

with our fast pacing updates I don't think that we really need to allow zf1f updates, and allowing the upgrade with a failing patch could be tricky too :-\

@ADDISON74
Copy link
Contributor

From the conversations I followed carefully, I think the two patches should be removed (I think this has happened) and a new version must be released. When OM it is installed with Composer, it will get the current ZF1-Future version, according to the version mentioned in composer.json.

Also, the current OM version is either completely removed, or we correct it with a ZF1-Future version up to X, as you also showed.

The less pleasant part is that we have to be careful when a new version of ZF1-Future is launched and not to approve the automatic the PR created by the bot without checking the existing patches.

@fballiano
Copy link
Contributor Author

The less pleasant part is that we have to be careful when a new version of ZF1-Future is launched and not to approve the automatic the PR created by the bot without checking the existing patches.

not really, because our composer requirement for zf1f allows the upgrade regardless we approve that automatic PR or not

@colinmollenhour
Copy link
Member

I agree that the version should be fixed given the circumstances. If there is a critical update, dependabot should tell us, right?

@fballiano
Copy link
Contributor Author

@colinmollenhour dependabot will tell us 1 week later (in the worst case)

thing is, composer validation is complaining not to use exact version number...

@colinmollenhour
Copy link
Member

Is it ok to use composer validate --strict --no-check-all?

@fballiano
Copy link
Contributor Author

I was checking that parameter earlier but the documentation is not super clear to me, anyway I think it’s our only way

@ADDISON74
Copy link
Contributor

I am not an advanced Composer user, but maybe we should ask for an opinion in their forum. I noticed that there are quite interesting discussions on various topics.

https://github.com/composer/composer/discussions

@fballiano
Copy link
Contributor Author

ok from https://getcomposer.org/doc/03-cli.md it's clear that the no-check-all is the parameter we want in this case:

--no-check-all: Do not emit a warning if requirements in composer.json use unbound or overly strict version constraints.

@fballiano
Copy link
Contributor Author

@colinmollenhour we will have to address this also in v19 because composer installation are broken in v19 at the same way...

@ADDISON74
Copy link
Contributor

Reading the documentation, we don't really have other options. What @colinmollenhour proposed would be the solution.

@fballiano
Copy link
Contributor Author

it's already implemented in this PR ;-)

@fballiano fballiano merged commit 10b63c1 into OpenMage:main Aug 31, 2023
19 checks passed
@fballiano fballiano deleted the fixversion branch August 31, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composer Relates to composer.json environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composer patches not applied successfully
5 participants