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
API Split SilverStripe\Assets into separate module #6723
API Split SilverStripe\Assets into separate module #6723
Conversation
ef2d9bf
to
dae6d59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 small change
@@ -23,6 +23,7 @@ | |||
"monolog/monolog": "~1.11", | |||
"nikic/php-parser": "^2 || ^3", | |||
"silverstripe/config": "^1@dev", | |||
"silverstripe/assets": "^1@dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a requirement of cms, not framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK while there's coupling still in place this is probably okay as a first step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there is quite a bit of work to de-couple that at this stage. It may be closer to a 5.0 / 4.late goal. :)
@@ -23,6 +23,7 @@ | |||
"monolog/monolog": "~1.11", | |||
"nikic/php-parser": "^2 || ^3", | |||
"silverstripe/config": "^1@dev", | |||
"silverstripe/assets": "^1@dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK while there's coupling still in place this is probably okay as a first step.
@silverstripe/core-team any objections? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will FileField
stay in framework?
I guess it's one of those cross-dependency/coupling issues you guys were talking about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections from me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the already mentioned coupling issues with some of the trickier parts of the system this looks like a great first step.
I feel like we've got a sufficient quorum so I'm merging! |
See https://github.com/silverstripe/silverstripe-assets
Fixes #5289
Requires silverstripe/silverstripe-installer#158