Ignore platform requirements object #46

boesing · 2021-08-17T08:10:25Z

Q	A
Documentation	yes
BC Break	no
New Feature	yes

Description

With one of the earlier versions, we introduced ignore_platform_reqs_8 to ignore PHP 8.0 composer dependency conflicts.
Since we will need this for PHP 8.1 as well, this PR adds a new way to ignore platform requirements for any PHP version.

New format now is:

{
    "ignore_php_platform_requirements": {
        "7.4": true,
        "8.0": false,
        "8.1": true   
    }
}

PHP 8.0 and PHP 8.1 are enabled by default or do we want to change default behavior for PHP 8.0 with the next release?

This PR also deprecates the usage of ignore_platform_reqs_8 in projects and will add a warning to the actions output.

Ocramius · 2021-08-17T08:21:24Z

PHP 8.0 and PHP 8.1 are enabled by default

@_@ what

boesing · 2021-08-17T08:52:38Z

Keeping BC 🤷🏼‍♂️
PHP 8.0 is still ignored platform reqs by default.
I am open to disable this and let projects enable that by themselves when adding PHP 8.1 support 👍🏼

Ocramius · 2021-08-17T10:31:05Z

If we started supporting ~8.0.0, we shouldn't have run with --ignore-platform-reqs on it :-\

boesing · 2021-08-17T11:33:54Z

Imho, having --ignore-platform-req=php for PHP 8.0 was the only reason why we were able to provide support for PHP 8.0 early on.
We have 160 packages which depend on probably even more packages (even tho, most of them are part of laminas) but if we could only provide PHP 8.0 support if the whole dependency tree made its changes to be compatible, we will end up waiting for months before we can start. Thus said, ignoring platform reqs was decent for providing initial support for PHP 8.0.

boesing · 2021-08-17T19:14:00Z

This will now output the following matrix when no further .laminas-ci.json is provided:

{
    "include":
    [
        {
            "name": "PHPUnit on PHP 7.3 with lowest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"7.3\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"lowest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        },
        {
            "name": "PHPUnit on PHP 7.3 with latest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"7.3\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        },
        {
            "name": "PHPUnit on PHP 7.4 with lowest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"7.4\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"lowest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        },
        {
            "name": "PHPUnit on PHP 7.4 with latest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"7.4\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        },
        {
            "name": "PHPUnit on PHP 8.0 with lowest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"8.0\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"lowest\",\"ignore_platform_reqs_8\":true,\"ignore_php_platform_requirement\":true}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        },
        {
            "name": "PHPUnit on PHP 8.0 with latest dependencies",
            "job": "{\"command\":\"./vendor/bin/phpunit\",\"php\":\"8.0\",\"extensions\":
            [],\"ini\":[\"memory_limit        = -1\"],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":true,\"ignore_php_platform_requirement\":true}",
            "operatingSystem": "ubuntu-latest",
            "action": "laminas/laminas-continuous-integration-action@v1"
        }
    ]
}

So while keeping BC compatibility by providing ignore_platform_reqs_8 (when the PHP version for the job is PHP 8.0), we also add ignore_php_platform_requirement for the very specific PHP version which is used to execute the Job.

If we want to merge this, I will prepare a PR within the CI action afterwards.

boesing · 2021-08-17T19:17:32Z

src/command.js

@@ -32,7 +32,8 @@ export class Command {
            extensions: this.extensions,
            ini: this.ini,
            dependencies: this.dependencies,
-            ignore_platform_reqs_8: this.ignore_platform_reqs_8,
+            ignore_platform_reqs_8: this.ignore_php_platform_requirement,


When running a job on PHP 7.4, it is absolutely fine to provide ignore_platform_reqs8 as false or true depending on the PHP 7.4 configuration.

The action itself will only apply this to the composer command if the PHP version used in the action is PHP 8.0 and thus, we are good to go.

This is good - it will ensure that the CI container doesn't need immediate changes, and we can update it after this is already released.

This is good - it will ensure that the CI container doesn't need immediate changes, and we can update it after this is already released.

Yup, that was the idea. This change is fully BC compatible.

boesing · 2021-08-17T19:18:30Z

This PR contains changes from #47

weierophinney

Overall, this is good. I'd like to get #47 merged first, so that the diff on the README.md file is less, before merging this one.

As to the comments by @Ocramius - this is about installing dependencies. There's a fair number of dependencies of ours that do not have their PHP constraints updated, though they work fine with it. By applying the --ignore-platform-req option, we can still test against them.

That said, we should likely start seeing if we can test without that flag as we start the PHP 8.1 rollout, though it will be tempered by needing it for 8.1 tests ... which this patch would allow for.

weierophinney · 2021-08-17T19:37:26Z

src/command.js

@@ -32,7 +32,8 @@ export class Command {
            extensions: this.extensions,
            ini: this.ini,
            dependencies: this.dependencies,
-            ignore_platform_reqs_8: this.ignore_platform_reqs_8,
+            ignore_platform_reqs_8: this.ignore_php_platform_requirement,


This is good - it will ensure that the CI container doesn't need immediate changes, and we can update it after this is already released.

…ected location Signed-off-by: Maximilian Bösing <[email protected]>

… message Signed-off-by: Maximilian Bösing <[email protected]>

…as-ci.json` The `ignore_php_platform_requirements` can be an object containing the PHP version to ignore along with the instruction to either ignore the requirements or not. This also deprecates the old `ignore_platform_reqs_8` configuration. Signed-off-by: Maximilian Bösing <[email protected]>

…requirement flags Signed-off-by: Maximilian Bösing <[email protected]>

boesing · 2021-08-17T19:52:51Z

index.js

@@ -10,7 +10,7 @@ import {Validator} from "@cfworker/json-schema";
 * Do early composer.json schema validation to avoid unnecessary ramp-ups of jobs which may fail
 * due to an incompatible composer.json.
 */
-if (fs.existsSync('composer.json')) {
+if (fs.existsSync('composer.json') && fs.existsSync('/action/composer.schema.json')) {


This is kinda unrelated but it always bit me when running this locally due to the absolute path requirement.
I can drop this again.

boesing · 2021-08-17T19:53:40Z

index.js

@@ -20,10 +20,7 @@ if (fs.existsSync('composer.json')) {

    if (!validationResult.valid) {
        validationResult.errors.forEach(function (outputUnit) {
-           core.error("There is an error in the keyword located by {0}: {1}".format(


This is also deprecated but when I tried to add https://github.com/laminas/laminas-ci-matrix-action/pull/46/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R48-R50, I had issues with String.format and thus, I wanted to ensure that this will definitely work.

weierophinney

One nit-pick, and thien this is ready.

README.md

…passed as PHP version Signed-off-by: Maximilian Bösing <[email protected]>

boesing added the Enhancement label Aug 17, 2021

boesing requested a review from weierophinney August 17, 2021 08:10

boesing marked this pull request as draft August 17, 2021 17:20

boesing mentioned this pull request Aug 17, 2021

Update README to reflect actual features #47

Merged

boesing force-pushed the feature/ignore-platform-reqs branch from f92abe4 to e283436 Compare August 17, 2021 19:00

boesing force-pushed the feature/ignore-platform-reqs branch from e283436 to ca4cf0a Compare August 17, 2021 19:14

boesing commented Aug 17, 2021

View reviewed changes

boesing marked this pull request as ready for review August 17, 2021 19:18

weierophinney reviewed Aug 17, 2021

View reviewed changes

boesing added 4 commits August 17, 2021 21:47

qa: only execute composer.json lint if the schema exists in the exp…

b6768fa

…ected location Signed-off-by: Maximilian Bösing <[email protected]>

bugfix: ensure we are properly formatting the schema validation error…

e16a08d

… message Signed-off-by: Maximilian Bösing <[email protected]>

docs: add more detailed documentation regarding the new php platform …

e62c34e

…requirement flags Signed-off-by: Maximilian Bösing <[email protected]>

boesing force-pushed the feature/ignore-platform-reqs branch from ca4cf0a to e62c34e Compare August 17, 2021 19:50

weierophinney added this to the 1.9.0 milestone Aug 17, 2021

boesing commented Aug 17, 2021

View reviewed changes

weierophinney requested changes Aug 17, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: rephrase additional_checks special behavior when wildcard is …

5cbbc0c

…passed as PHP version Signed-off-by: Maximilian Bösing <[email protected]>

weierophinney approved these changes Aug 17, 2021

View reviewed changes

weierophinney merged commit 059424a into laminas:1.9.x Aug 17, 2021

boesing deleted the feature/ignore-platform-reqs branch August 17, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Ignore platform requirements object #46

Ignore platform requirements object #46

Uh oh!

boesing commented Aug 17, 2021 •

edited

Loading

Uh oh!

Ocramius commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

Ocramius commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

boesing Aug 17, 2021

Uh oh!

weierophinney Aug 17, 2021

Uh oh!

boesing Aug 17, 2021 •

edited

Loading

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

weierophinney left a comment

Uh oh!

weierophinney Aug 17, 2021

Uh oh!

boesing Aug 17, 2021

Uh oh!

boesing Aug 17, 2021

Uh oh!

weierophinney left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ignore platform requirements object #46

Ignore platform requirements object #46

Uh oh!

Conversation

boesing commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Ocramius commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

Ocramius commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

boesing Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

weierophinney Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

boesing Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boesing commented Aug 17, 2021

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

weierophinney Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

boesing Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

boesing Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

boesing commented Aug 17, 2021 •

edited

Loading

boesing Aug 17, 2021 •

edited

Loading