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

ENH CMS 6 compatibility #49

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 4, 2024

Issue silverstripe/.github#350

Needs silverstripe/supported-modules#49 merged before CI will work

Changes silverstripe/closure dependency to laravel/serializable-closure

Also a bunch of other maintenance changes to make this compatible with a CMS 6 install to ensure things work correctly in CI

Create a 5.0 branch after this is merged and tag 5.0.0-beta1, though we could arguably just release it straight to stable

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

I assume this needs silverstripe/supported-modules#49 to go green....

Though the error is "No valid PHP version allowed". I thought it used the php dependency from composer.json as a fallback? Seems we should definitely investigate that and fix it if needed rather than arbitrarily dropping this into the supported modules json.

README.md Outdated Show resolved Hide resolved
@emteknetnz
Copy link
Member Author

I thought it used the php dependency from composer.json

It would, expect this module does not actually require silverstripe so CI cannot just look at the version of framework in composer.json

@GuySartorelli
Copy link
Member

GuySartorelli commented Dec 5, 2024

I thought it used the php dependency from composer.json

It would, expect this module does not actually require silverstripe so CI cannot just look at the version of framework in composer.json

What does the version of framework have to do with it? The composer.json file for this repo has php as a dependency explicitly in the require section.

Edit: I've just had a look and isAllowedPhpVersion does check the composer php constraint before anything else.
I'm hesitant to merge silverstripe/supported-modules#49 because it seems like it's just working around a legitimate bug in generate matrix.

@emteknetnz
Copy link
Member Author

I think isAllowedPhpVersion() checks to see if a particular version of php is allowed given the current composer.json being used

Before that a list of valid PHP versions is generated, e.g. for cms 6 it is 8.3 and 8.4

However doorman is not aware of what version of the cms is being used, because there isn't a version of framework, or admin etc in composer.json which can be used to derive the version of installer to use

Hence it's given the error "No valid PHP version allowed", cos the list of allowable PHP versions is empty

@GuySartorelli
Copy link
Member

Ahhhh yup you're right. Probably that's something we ought to change but not right now.
Eventually getListOfPhpVersionsByBranchName will swap to using 5 as a fallback (or MetaData::LOWEST_SUPPORTED_CMS_MAJOR which will also swap to 5) and that will use more modern PHP versions.

For now, the change you've made is correct.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

MOG

Create a 5.0 branch after this is merged and tag 5.0.0-beta1, though we could arguably just release it straight to stable

Wouldn't it be alpha1 not beta1?
In any case I think since this isn't a supported repo it doesn't need to go through the alpha->beta->rc->stable cycle. Feel free to tag as stable if you think that's more appropriate.
We might forget to tag the stable if we don't do it right away so that's probably best.

@GuySartorelli
Copy link
Member

Matrix still refusing to build

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 9, 2024

I've created a PR which should resolve silverstripe/gha-generate-matrix#115

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit fd40c43 into silverstripe:5 Dec 9, 2024
9 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/closure branch December 9, 2024 21:44
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 this pull request may close these issues.

2 participants