robbieaverill
commented
3 years ago I'm into it, granularity is a good thing in this space! One day using Doctrine instead might be feasible too.
Open sminnee opened 3 years ago
robbieaverill
commented
3 years ago I'm into it, granularity is a good thing in this space! One day using Doctrine instead might be feasible too.
sminnee
commented
3 years ago I've started looking at this, there's a bunch of small tidy ups to make, and this will also require splitting out test-support.
I'm thinking that I'll get some PoCs of these two packages set up on sminnee. I won't bother about preserving history for this stage, but I'll track the changes I need to make to the files, and where viable raise them as PRs on the current project. (Eg replace Director::is_cli with the equivalent on Environment)
Once I've got some PoCs on sminnee I can report back here with the changes that are needed to get to it all working.
For the first cut it won't be "truly" decoupled in all areas - there will still be a fair number of class_exists checks - but it will at least highlight the gaps.
sminnee
commented
3 years ago OK the PoC is here: https://github.com/sminnee/silverstripe-core-poc, complete with a passing 1.8s test suite :-).
Some observations:
Our subsystem decomposition is, unsurprisingly, a bit messy and incomplete. Systems are haphazardly/accidentally bound together. For example, Application is ostensibly a class that isn't coupled to our Controller stack, and yet subsystems that reference Application also assume the presence of our HTTPRequest class. So it's coupled to Control. I believe that it's better to get subsystems right, and I also want to avoid a bunch of class renames, but I think this is going to mean a lot of mis-filed classes in _legacy paths. My view is that's an acceptable trade-off. I haven't gone the next step of deprecating these shuffled classes but that would be a logical next step. I don't think it's essential to the partitioning work, so I'm leaving it for now.
Correct subsystem decomposition is hard, which is why we've got gaps. My current thinking is that I probably want to roll out the some of the other components as PoC modules like this core one, and see how stitching them together in different ways works out in practise. I don't think a PR and a bit of peer review will be sufficient to get it right, and changing decomposition after we made a slide is possible by a bit fiddly.
Some of the gnarliest pieces of the bootstrap - like the logic around flushing - are really part of the Control subsystem, as they relate deeply to 'how can I safely execute this administrative function based on publicly submitted URLs'. Should this be part of Core? Probably not... unless these provides base support some features that will remain essential if we connect to another Controller. I think that trying to connect this to another controller as part of those PoC setups will be good.
Dev is a bit of a mess. I'm really set on ensuring that we don't have cyclic dependencies (ie. avoid core requires dev and dev requires core). But I'm also keen to allow the use of new versions of PHPUnit in your projects without forcing an upgrade of core. I think that where this will probably land is that silverstripe/core provides the classes for test state management, and avoids using a SapphireTest base class itself, instead being refactored to a more composable approach.
As such I think next steps are:
sminnee
commented
3 years ago Another question: should silverstripe/core provide i18n? it will be a dependency of View and ORM, arguably it's part of the 'base layer' of the platform. For my PoC I've opted to include it, but it could equally be split into its own package. I'm not sure that splitting it out as its own package adds a great deal of value, however... Keen to hear reasons why it might be good to split it out.
Also keen to hear from @dnsl48 @Cheddam @bergice @emteknetnz @maxime-rainville about the release management impact for adding more modules, and the validity of the assumption that some of the base layer modules are less likely to need high-volumes of changes.
emteknetnz
commented
3 years ago the release management impact for adding more modules
I'm pretty inexperienced on the release side of things, though from what I've seen so far I'd say there will be a bit of initial setup involved, though after that not too much extra work given we're already tooled up to release a large number of modules in the CWP release process.
base layer modules are less likely to need high-volumes of changes.
I think this may be kind of irrelevant, the vast majority of modules releases are done during the quarterly release. We only do a small handful of individual module releases outside of this for high impact bugs.
One other thing that is worth considering is how (or even if) we'd go about migrating the backlog of github issues on silverstripe-framework to the correct new repo.
Overall I am certainly curious about this approach and the possibility of doing ^4 || ^5 in composer.json to allow us to do incremental major upgrades. It does seem like a pretty sane way to move things forward.
sminnee
commented
3 years ago Cool, that's encouraging.
One other thing that is worth considering is how (or even if) we'd go about migrating the backlog of github issues on silverstripe-framework to the correct new repo.
The move issue feature doesn't break GitHub any more, so we can use that. Although it's not necessary (bugs mis-listed in framework will still get the necessary attention) it would be nice to correct them in this way. It would make it easier, to say, focus only on the ORM issues/PRs, which might get more interest from selected community members who can't be across the whole core recipe.
PRs are more challenging, they require rework a per-PR basis, more or less manually. It would be worth a bit of a push to clear up any PRs in these areas before we actually do this split for real.
sminnee
commented
3 years ago OK the 2nd extracted PoC is here, silverstripe/orm: http://github.com/sminnee/silverstripe-orm-poc
Notable cross-module ties that are a little challenging to deal with (for now they don't work, and skip tests, unless you install the relevant modules:
I think I want to do give the same treatment to Forms (sans GridField), Control and View before considering whether we have the split correct. Thankfully, it's not overly time-consuming to put these PoCs together.
In general, however, so fair the modules have become less clouded by distractions and I believe the test suites are also faster, for example, without the concerns of the Security module cluttering the ORM testing. And although I've had to throw a lot of code into classmap loading (there are 14 such classes in the orm package), to the best of my knowledge no APIs have been broken.
michalkleiner
commented
3 years ago I'll certainly keep my eyes on this topic. As much as I'm for separation of concerns and granularity etc., we also experienced what amount of work and troubleshooting it was to upgrade from 3 to 4.
It also finally started to feel like 4.x is reasonably settled, stable and we know how to work it efficiently.
I'm not sure how seamless this can eventually end up being (I do notice the effort), but I'm pretty sure our clients won't stomach another "Silverstripe changed things (again), can we have another 70k to upgrade your site (to be functionally the same as it was) so you can continue to receive security updates?" request.
sminnee
commented
3 years ago Yeah the goal is to have the same functionality (and classnames) moved into different composer packages, so I'd say the upgrade risk is relatively low.
I'm acutely concerned of the impact of such upgrades, and the importance of avoiding upgrades that are both expensive (lots of breaking changes at once) and forced (the main driver for the upgrade was EOL of the previous version).
One of the challenges is to keep things stable without simply giving up on changing things. Being able to experiment with smaller, more tidily decoupled packages, is likely to be of benefit, which is my motivation.
TL;DR: "yes, I agree, that's why I'm proposing this."
If this gets progressed (I would not expect it to drop in the next minor release tbh) it will need beta testers; I'll ping you to make sure we're not breaking thing on your sites once it hits a stable tag ;-)
dnsl48
commented
3 years ago release management impact for adding more modules
Yes, as @emteknetnz said, there are absolutely no troubles from having some more modules regarding the release management.
should silverstripe/core provide i18n? it will be a dependency of View and ORM, arguably it's part of the 'base layer' of the platform. For my PoC I've opted to include it, but it could equally be split into its own package. I'm not sure that splitting it out as its own package adds a great deal of value, however... Keen to hear reasons why it might be good to split it out.
The main reason for splitting it out may be the same as for ORM and View - decoupling. Since the aim is 4.x branch, we most likely won't be able to bring in lots of new APIs and get rid of hard dependencies in the code. That means, most likely every composer module will require almost every other composer module anyway. However, if we start doing that for some modules, that's a good opportunity to go "all in" and apply the decision for all components we've got, so later we don't have "exceptions" on our hands, and any further decisions will scale more naturally for every decoupled module.
sminnee
commented
3 years ago I might draw some diagrams of what I've discovered so far.
maxime-rainville
commented
3 years ago I don't think it will be problematic for releases. It's really the core modules that are adding a lot of time to releases ... it's all the "satellite" feature modules.
Even if it's not possible to properly decoupled all the modules in the SS4 release line and we end up with a bunch of inter-dependants modules, I still think it's worth doing. It would reduce our build times and it would force us to properly separate concerns when working on a framework offshoot.
sminnee
commented
3 years ago OK, as promised, a couple of diagrams. So far, I've PoCed core & orm, and I've started on View. I'll update the diagrams once I've taken the View, Control, and Forms split a little further.
The source of the diagrams is here but unfortunately it's locked inside the SS ltd google drive https://docs.google.com/presentation/d/1JYLPXM5-bcWmNmtm01HbgzwFT5tmYWPyRkMxe3M6G7s/edit?usp=sharing
sminnee
commented
3 years ago Some things that are still a bit awkward:
Resource URL generation depends on interfaces that are provided by assets. Which means that you need to bring the ORM, File/Folder tables, etc etc just to expose the public URL of a javascript file in a module. I would probably bring these interface into core (since core's ModuleResourceLoader requires it), and provide the implementations in the Controller system, since that is responsible for serving URLs. Different frameworks could be supported by introducing another ResourceURLGenerator implementation.
Generated assets requires GeneratedAssetHandler, provided by the asset module. This is a dependency of both View (for combined requirements) and Forms (for TinyMCE configuration handling). Again, this might be something where Core provides an interface, Control provides a basic implementation and Assets provides a full-featured one. It's basically a system for writing a file somewhere and then supplying its URL, which is where the Control tie-in makes sense.
RateLimiter requires the current time. Responsibility for "now" sits with DBDatetime as it has a mockable now() static method. I would probably deprecate this for a mockable current-datetime facility in core somewhere.
PaginatedList in ORM need to return modified URLs. It currently uses HTTPRequest and HTTP::setGetVar to do this. Potentially it could make use of a simpler interface that provides methods to return a URL with get vars modified. This might be a bit over-engineered though.
chillu
commented
3 years ago Exciting! Great to hear that the release managers (CMS Squad) don't think this will significantly impact the maintenance effort. Of course, if we do perform an orm@2 release in addition to maintaining orm@1, then we have more combinations to test and maintain. Which is currently "avoided" because framework@5 is too big of a jump. But this is a good problem to have (iterating on smaller units).
Developers familiar with Silverstripe CMS are working on other projects and need a data store. I had a web spider project that I wanted to pull a database into, and tried this. It went okay until framework pulled in assets and my project all of a sudden required a filesystem store.
So to clarify, that's without needing a CMS? Without that aspect, we have a pretty steep battle against Laravel etc, but I take the point about leveraging existing Silverstripe knowledge.
Developers working on a Silverstripe site want to access its data with another project. So the front-end of a website could pull in a project's models and connect them to a completely different front-end system.
That's the key use case for me: We need to reduce lock-in. Just because we happen to build a great CMS and ORM doesn't mean we're always the right choice for frontends. And our multi-table inheritance makes it quite hard to get data out without relying on Silverstripe PHP - raw queries are more of a liability than they are in your average Laravel/Eloquent project.
But that's also where the proposal is the weakest: All the other ~2000 Silverstripe modules (incl. core) are still going to have require: cms in their dependencies, and will force you to pull in everything anyway. As will any model definition with getCMSFields() = referencing FormField, potentially Session, which needs the "view" layer, and I'd be very surprised if we managed to untangle the view/controller layers.
So if my goal is to build a PHP website with Laravel views and forms, data access through a Silverstripe ORM, then how do I end up with less dependencies in my project after we've decoupled the ORM and other modules?
And if my goal is to run serverless PHP functions with a lightweight API layer, I'm still stuck deploying 50MB of source code packages to AWS, they're just in different folders now.
So concluding, I think we need to be clear about the goals: Decoupling is always a chance to refactor and focus, and this will hopefully benefit the long-term maintenance. And the possibility on iterating on orm@2 without a framework@5 is definitely attractive. But can somebody clarify how this would lead to less dependencies being installed for the use cases described?
sminnee
commented
3 years ago But that's also where the proposal is the weakest: All the other ~2000 Silverstripe modules (incl. core) are still going to have require: cms in their dependencies, and will force you to pull in everything anyway.
My hope is that these can incrementally refine their dependency references to be more specific, if & when this decoupling was released. It's frequently incorrect, yes, but it's also a small PR.
More speculatively, there's an option for CMS to define admin as a suggested, rather than required, dependency, letting you pull the SiteTree models into a project without the admin baggage.
As will any model definition with getCMSFields() = referencing FormField
My plan (and I want to see what it looks like in a PoC before doubling-down on it) is that you'll be able to install your model without Forms, but thing that would normally call getCMSFields() will be stopped. So, for example, you could get a GraphQL server on your data going but it wouldn't expose any form schemas. Forms would be a suggestion of ORM rather than a requirement.
More speculatively, I'm keen to see whether Controller can be a suggested, rather than required, dependency of Forms, so that you could do things like FormSchema-in-a-GraphQL response but not deal with the actual response handling of a Controller.
potentially Session
Session needs the controller layer not the view. It would be interesting to see how common direct session references are in getCMSFields(),
I'd be very surprised if we managed to untangle the view/controller layers.
I'm naïvely attempting this. ;) View is half-done, I haven't touched Controller yet. My sense is that most of Silverstripe's most awkwardly-entangled code (including the state management necessary to secure ?flush) is really part of the Controller layer.
But can somebody clarify how this would lead to less dependencies being installed for the use cases described?
I think building a PoC is the best way to clarity. Trying to reason though this in a GitHub issue is next to impossible, and a lot of my preconceptions prior the PoC work I've done so far were disproven once I started throwing around code.
sminnee
commented
3 years ago So to clarify, that's without needing a CMS? Without that aspect, we have a pretty steep battle against Laravel etc, but I take the point about leveraging existing Silverstripe knowledge.
It's potentially without needing a ModelAdmin but maybe needing to add one later. I've had the following loop in projects once or twice, I'm not sure how much of a pattern it is:
Silverstripe as it currently stands forces the bullet points to be reversed :p
maxime-rainville
commented
1 year ago I'm thinking we should re-target this to CMS5.
This idea has been discussed over the years but there are two things I really want to highlight:
Why split out core and orm?
My initial goal would be to split out core. On its own, however, that's not hugely useful. Splitting out ORM as well as the first major task provides some that is of concrete use.
Following that I would recommend splitting out Control and View, and subsequently Security, Forms, and probably GridField.
What inaccuracies in subsystem decomposition?
ViewableData is currently in the View module. This is incorrect. It's the base object of many Silverstripe objects and as such it's part of core. If you don't think we should have an object such as this at all, I tend to agree, but everyone's code expects it, and even if you provided all the methods you'll still break any
instanceof ViewableDatachecks.Mistakes in our subsystem decomposition have led to subsystems being heavily interdependent, meaning that even where they are broken into separate composer packages and git repos, releases have to be carefully coordinated and release management is much more complex.
But the namespaces!
From my perspective, it's more important that we get the subsystems right than we get the namespaces to neatly map to subsystems. Ideally we have both, but in order to preserve CMS4 compatibility, we have to choose. As such ViewbleData would retain its current namespace / FQCN and be loaded with classmap.
ViewableData.php would probably live in
/src/_legacyor a similar pattern established by @unclecheese's current work on maintaining graphql 3 & 4 support simultaneously in asset-admin.Would we preserve history?
Yes. You can process the framework repo to capture the history of the Core or ORM subfolder and create a new repo based on that. It's less clear as to whether the ViewableData history could be preserved, but there are some things we can try.
Won't more modules make release management harder?
Maybe. But the idea is that some packages will evolved much more slowly than others.
Here's a rough count of the non-merge commits of different parts of the system over the past year:
It's likely that a release of new features to Forms, ORM, or Security will be what is most relevant, and that other packages are likely to move more slowly. Not every package in the recipe needs a tag with every recipe updater (I believe we dropped that habit a while back).
It also provides the possibility that people (particularly people who aren't full-time on the project) might be able to act as greater contributors on individual modules rather than the whole suite. Of course, I don't think that this will happen by itself, but it's something we can look to encourage.
Silverstripe 4, you say?
Yes. As long as silverstripe/framework requires silverstripe/core, it's providing all the same classes. The price we pay for this is some classmap oddities. However, I think that's a small price to pay to be able to benefit from this refactoring now. And if we want to, we can always make it better in ORM 2.
What happens after that?
It opens the possibility of jumping another major release just for one of the packages, rather than the entire framework monolith, which I think will be important for experimentation.
It also lets us use the ORM as a lean data store without the rest of the framework in place. This would be helpful if: