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

fix(manager/composer): match composer's handling of the patch number for platform PHP version #17299

Merged
merged 18 commits into from Aug 29, 2022
Merged

fix(manager/composer): match composer's handling of the patch number for platform PHP version #17299

merged 18 commits into from Aug 29, 2022

Conversation

internalsystemerror
Copy link
Contributor

@internalsystemerror internalsystemerror commented Aug 20, 2022

Changes

When specifying a deliberately high patch version (i.e. a never to be released version) e.g. 7.4.99 for config.platform.php inside composer.json, this is currently handled by composer to imply the latest available patch version. This PR brings renovate in line with that usage, by replacing any patch version number greater than 0 with <=, (i.e. 7.4.99 becomes <=7.4.99 for the purpose of resolving the docker tag).

Context

At present when users are specifying a non-existent patch number, this tells composer to retrieve updates using the latest available patch for that PHP minor. Although not ubiquitous (as some will want to specify a specific patch version), nor a written standard (composer documentation only mentions the case where no patch is specified, which is treated the same as patch 0 (i.e. 7.4 is taken to be 7.4.0).

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required
    Although this is a change to renovate, we are using a feature of composer, and this PR simply brings it into line with that usage. However, I'm happy to include documentation changes if someone could point me to what and where.

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

There will still be examples where a version such as 7.4.50 is specified, which is a non-existent version. In this case, composer will use 7.4.30 (latest patch for minor) whereas renovate will request the docker tag 7.4.50 which doesn't exist and so will fall back to the latest tag (which is 8.0 currently).

In our discussions it had been suggested to drop the patch version altogether as we couldn't think of a case where the latest patch version wouldn't be allowed, quite the reverse, this is usually to require a minimum version that includes a specific fix. However, as we did not want to prevent users from being able to specify a patch version, a patch of >=99 appeared to be a suitable compromise. After further discussion in the issue above, the approach will now be to replace any patch version number greater than 0 with <=, (i.e. 7.4.99 becomes <=7.4.99 for the purpose of resolving the docker tag).

@internalsystemerror
Copy link
Contributor Author

internalsystemerror commented Aug 20, 2022

Some points regarding, whether a PHP version with a patch of .99 would ever be released:

Latest patch versions...
5.6.40
(there is no v6)
7.0.33
7.1.33
7.2.34
7.3.33
7.4.30 (security-only)
8.0.22 (actively maintained)
8.1.9 (actively maintained)

@rarkins
Copy link
Collaborator

rarkins commented Aug 21, 2022

Sounds like composer treats a plain version as like <=, would it be better or we did that too?

Eg we treat 7.4.99 as <=7.4.99

@Xerkus
Copy link

Xerkus commented Aug 21, 2022

Composer treats 7.4 as 7.4.0. For packages defining php dependency as say ^7.4.1 (a shorthand for >=7.4.1 <8.0.0) platform defined as 7.4 would not be compatible.

So, to say we have the latest of the 7.4.x we use arbitrary high patch number (platform version must be specific. It can't be a range) to ensure platform version will satisfy packages requiring any of the existing releases as their minimum. Packages require specific PHP patch version as minimum when there was a php bug affecting the package that was fixed by a particular patch version. I never seen package that will not accept latest patch version.

Composer itself does not have any special treatment to the number. It just needs to be within the range for the purposes of dependency resolution. I've seen .99 and .999 used most often for that purpose.

My understanding is that the issue we experience comes from the fact that 7.3.99 is not a real version and renovate does not have a matching docker tag, falling back to :latest and using wrong php version altogether.

These problems occurred while renovating this repository.

  • WARN: Failed to find a tag satisfying constraint, using "latest" tag instead

Eg lock file maintenance laminas/laminas-skeleton-installer#39 creates lock file against package versions that require php 7.4 as a minimum where defined platform and expected lock file is for php 7.3.
Composer itself deals with 7.3.99 defined in platform properly.

@internalsystemerror
Copy link
Contributor Author

internalsystemerror commented Aug 21, 2022

Composer allows the use of config.platform.php in order to override the detected PHP version and specify the specific PHP version to use when fetching dependencies. We are reading from this setting in order to override the specific PHP version used for renovate.

The difference currently lies with how composer vs renovate handles a non-existent version. Composer will obtain packages using 7.4.99 as the PHP version matching packages that require "php": "^7.4.27" and "php": "<8.0.0".

Renovate however, passes this as a docker tag to fetch the correct PHP version. The ideal scenario would be to check if the patch version exists and if not, drop the patch number and check again.

Specifying a setting of "7.4" in composer.json is treated by composer as "7.4.0". This currently differs from renovate too, and now I think about it, I may wish to amend this PR to set the patch number to 0, if there is none (now handled).

Also note we're talking specifically about the config.platform.php setting which is a specific version used only when the root composer.json to override a PHP range in require.

See https://getcomposer.org/doc/06-config.md#platform for composer's documentation on this feature.

@rarkins
Copy link
Collaborator

rarkins commented Aug 21, 2022

I think we need to discuss this in an issue to reach consensus on how Renovate should treat these edge cases, including composer's undocumented behaviour. @internalsystemerror Could you create one and we move the dissuasion there?

@internalsystemerror
Copy link
Contributor Author

internalsystemerror commented Aug 21, 2022

It's not that the behaviour is undocumented, this is how composer treats the version specified in this setting. 7.4 means 7.4.0 (different from docker tags) and a non-existent version number is used as is (again different from docker tags).

I've opened the following issue #17308 anyway. Hopefully this PR will bridge the differences whilst not having to make multiple round trips to docker in order to resolve the correct docker tag to use.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the discussion? link do some docs/ source code about composer behavior

lib/modules/manager/composer/utils.ts Outdated Show resolved Hide resolved
@internalsystemerror
Copy link
Contributor Author

where is the discussion? link do some docs/ source code about composer behavior

In the comment above, I opened an issue for discussion. I've also provided a comment to try and clearly explain the current error with examples, and then a further comment to show why this doesn't occur with e.g. node.

The issue again for brevity: #17308

@viceice
Copy link
Member

viceice commented Aug 21, 2022

please move the issue link to context section and add - to the issue, so the issue title is visible.

@internalsystemerror
Copy link
Contributor Author

internalsystemerror commented Aug 21, 2022

add - to the issue, so the issue title is visible.

Ah! I didn't know that was a GitHub feature, glad I learnt it. Done!

@internalsystemerror internalsystemerror changed the title fix(manager/composer): handle patch version .99 for platform PHP version fix(manager/composer): match composer's handling of the patch number for platform PHP version Aug 21, 2022
@Xerkus
Copy link

Xerkus commented Aug 21, 2022

For the docker latest tag replaces previous. It makes sense that builds tagged with 7.4 will be replacing previous builds and end up meaning latest build (and release)

In the context of platform overrides we talk about specific versions so it can not be expanded with anything implied other than .0.

link do some docs/ source code about composer behavior

For the constraints the Stability Constraints indirectly covers implicit behavior: https://getcomposer.org/doc/articles/versions.md#stability-constraints

…for platform PHP version

Signed-off-by: Gary Lockett <gary@creativecow.uk>
@internalsystemerror
Copy link
Contributor Author

internalsystemerror commented Aug 22, 2022

After updating using the solution agreed upon in the issue linked to this PR, I rebased and squashed to a single commit. However, you guys update quicker than I can keep up so it's already out of date again.

Consider this PR now ready for review.

viceice
viceice previously approved these changes Aug 22, 2022
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

@internalsystemerror
Copy link
Contributor Author

I assume you guys are able to update the branch prior to merge, but if not just let me know.

@viceice viceice requested a review from rarkins August 22, 2022 11:14
@internalsystemerror
Copy link
Contributor Author

@rarkins sorry for the ping, I know your time is precious but a) I wanted to make sure that this is on your radar, and b) I wanted to ask if there are any other changes needed from me, or if I just need to wait patiently?

lib/modules/manager/composer/utils.ts Outdated Show resolved Hide resolved
lib/modules/manager/composer/utils.ts Outdated Show resolved Hide resolved
@rarkins rarkins enabled auto-merge (squash) August 29, 2022 06:21
@rarkins rarkins merged commit 94cfaec into renovatebot:main Aug 29, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.181.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renovate handles config.platform.php different from composer
5 participants