DEP Silverstripe CMS 5 compatibility #779
Conversation
These methods are not in use and have been deprecated since fluent 5.
|
Added one last commit to remove deprecated methods which were deprecated in fluent 5 and aren't called in any of the code in 6 or 7. The removal of these methods will be highlighted in the upgrade guide, when I write it. |
|
Not sure why scrutinizer is angry (can't install npm 18 for some reason?), but @tractorcow unless you've been regularly checking it I'd recommend either removing it entirely or, as a less drastic approach, accept the scrutinizer failure for this PR and handle that separately. |
| }, | ||
| "scripts": { | ||
| "build": "NODE_ENV=production webpack -p --bail --progress", | ||
| "build": "yarn && yarn lint && rm -rf client/dist/* && NODE_ENV=production webpack --mode production --bail --progress", |
There was a problem hiding this comment.
Why is the rm command present? I don't see this in any other modules, for example https://github.com/silverstripe/silverstripe-linkfield/blob/1/package.json#L20
There was a problem hiding this comment.
linkfield isn't a supported module.
Look at any supported module's CMS 5 compatible version, e.g. https://github.com/silverstripe/silverstripe-subsites/blob/3.0/package.json#L4
It's to protect against old files from earlier builds sticking around (e.g. as a result of webpack copy), or from people putting things in dist manually. We should always be able to blow away dist and rebuild and nothing will be lost - this ensures that's always the case.
| $newState->setLocale($locale); | ||
|
|
||
| $records = LocalisedParent::get()->sort(...$sortArgs); | ||
| if ($useOrderBy) { |
There was a problem hiding this comment.
[nitpick] Use ternary operator.
There was a problem hiding this comment.
That's subjective, so unless there's some specific standard that this module adheres to which requires the ternary here I'm inclined to not make this change in this PR.
There was a problem hiding this comment.
Agree, hence the nitpick flag. Happy to keep it as is.
|
Left some comments but the changes look good so far 👍 |
|
Just approved the build to run. |
|
Merge at will. let me know if you need help releasing /etc. |
|
I just:
|
|
Nice job :D |
Replacement for #777
Rebased on master. This is the same code that is in the beta, but with references to
silverstripe/silverstripe-fluentreverted back totractorcow/silverstripe-fluentCI is green in our fork: https://github.com/creative-commoners/silverstripe-fluent/actions/runs/4190234023
After merging
Ideally this would be tagged
7.0.0-beta1- we can then (potentially) update our recipes to point back here instead of the fork - but even if we don't, it gives people the option of installing the beta of this module with the correct composer name.Parent issue