-
Notifications
You must be signed in to change notification settings - Fork 578
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
Twill Capsules #729
Twill Capsules #729
Conversation
👏 First thing I'd like to say is that I think a little more structure around Twill components, modules.etc. is a good thing. However, I am curious whether this solution is actually solving a problem, or making things more complicated? My pet peeve right now with Twill is the over-reliance on configuration and array structures (hello, wordpress! Which is so easy to get wrong, especially considering the documentation is lacking. It also means you mean more checks in Twill code!!). This PR seems to make that problem worse. I think we should move more strongly toward built-in PHP structures and concepts rather than trying to solve them with arrays. For example, it would be amazing if Twill had auto-discovery enabled for all its relevant classes and structures, rather than having to configure it all separately. Finally, some of the folders in the proposed structure seem to be redundant (ie. do we really need a second "app" folder?) and what about merging data models/repositories? Using class suffixes is generally redundant if they're not sitting in the same folder. Ie. PostRepository in Repositories, works just as well as PostRepository in Data, where it is not likely to have a clash. This would flatten folder structures and actually make the code simpler to read, albeit minorly. I actually quite like how Twill is structured now - it fits right in with my existing application. However, I can see with existing or legacy applications that this might actually be a problem, so capsules does solve that problem at least, and as you said - makes copying and pasting capsules between projects an absolute breeze, which is certainly very cool and useful (also makes writing packages easier). In conclusion, I think this is the right path forward but doesn't go far enough :) |
Str::contains($class = get_class($this), 'Models\Translations') && | ||
property_exists($class, 'baseModuleModel') | ||
$this->isTranslationModel() && | ||
property_exists($this, 'baseModuleModel') |
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.
If we used an interface for translatable models, we wouldn't need to do these sorts of checks.
The idea behind the current Capsule directory structure is to make a it look like "small Laravel application", so that's why it has app, database, resource and routes as root folders. At some point a Capsule structure can get complex, and I struggled organising my packages folders before and some of the refactors made me slowly go with a Laravelish structured folder for them too, and, I also believe you don't have to think too much to find things in your Capsule, just use the muscle memory you already built to manage your Laravel applications. But, of course I'm sure we can agree on a better structure if we can find one that serves everyone. I agree it doesn't solve a huge problem, but reuse modules is hard with Twill today, and IMO, when you have a lot of modules you have a lot of files (not only the ones Twill will create, but you will probably create more to support that module), and when those files are spread over a bunch of different folders, and some no even on the same root folder (like resources), at some point you will waste some time looking for the needle. If you need to rename something, for instance, (naming is hard, but renaming is much harder), good luck finding all places where you're using that thing. The other problem it can solve in the future is let the community build downloadable Capsules:
As it won't be easy to extend Capsules, maybe it could just copy the files to your /Capsules folder and allow you to do whatever you want with them. But it could also be a nice thing to be able to extend a Capsule and get future improvements, which is possibly a bad idea if we look to projects like Drupal, where the community made a huge mess building too much modules and not keeping them maintained. Moving blocks config from twill.php to /blocks, is something that I also did because having to get back to arrays over and over to configure them is painful (I don't even like Laravel's array's config files, PHP arrays are too verbose, on my personal packages I'm using YAML as much as I'm able to), so I agree we could move to make things a bit more automatic, build awareness around Twill's classes. But the array key (on twill.php) introduced to configure Capsules, was necessary for people to be able to disable them. We can probably make it optional, discovering and booting all Capsules in the /Twill/Capsules folder, and use the config only if present, but I believe it's necessary otherwise we would have to tell people to remove files from that folder in order to not discover them. About the class naming, I could change too much of Twill's current class naming system because Capsules are using internal Twill's structure exactly the way it is today (a few things were changed on the original code), this was done because it will be BC and fully optional. Again, I agree we can move the Repository and Model to /Data to simplify the structure (in projects I fully control I DRY class naming too), but if more Models are needed on someone's Capsule, more files will ended up landing on that folder. Thanks for your time and words, and we can change anything that can improve it, it's just a draft after all and you are probably the first person to review it. ❤️ |
Fair points. I would be wary however of making something laravelish, without really having to. I do think the app folder is redundant, and possibly even confusing, especially for new users. |
/rebase |
Capsule directory structure refactored to be this by default:
I say "default" because the structure and namespaces are (and were already) fully configurable: return [
'path' => app_path('Twill/Capsules'),
'namespaces' => [
'subdir' => '',
'base' => 'App\Twill\Capsules',
'models' => 'Models',
'repositories' => 'Repositories',
'controllers' => 'Http\Controllers',
'requests' => 'Http\Requests',
],
'list' => [
// ['name' => 'Artists', 'enabled' => true],
// ['name' => 'Posts', 'enabled' => true],
],
]; |
Just pushed a first version of the install command. For now it's only installing (--copy) by downloading a zip of the repository and unzipping it into Next I see for this command would be to be able to require (like a PHP Composer) a capsule what could be updated at every Here's the command output:
As always, everything is configurable, to allow downloading it from other Git servers/services, branches, vendors...: protected $signature = 'twill:capsule:install
{capsule : Capsule name (posts) in plural, Github repository (area17/capsule-posts) or full URL of the Capsule git repository}
{--require : Require as a Composer package. Can receive maitainer updates.}
{--copy : Copy Capsule code. Cannot receive updates.}
{--branch=stable : Repository branch}
{--prefix=twill-capsule : Capsule repository name prefix}
{--service=github.com : Service URL (defaults to Github)}'; |
/rebase |
Capsules are modules 'containerised' into a single folder. Nothing really changes except that we can copy and paste a whole module to another project it will just work, as soon as you enabled it on config (
twill.capsules
).Next for Capsules is to allow users to install capsules from Git repositories:
AREA 17 will probably have a repository with some opionated Capsules, but the community will be free to publish more.
But you can already, of course, use the command
To create a Capsule and you'll get this (proposed) structure:
But users can easily change the namespace structure on config:
Tests are passing for PHP 7.3 and 7.4, but still needs work for other versions.
Seeders are allowed but require users to add a trait to the DatabaseSeeder and invoke Capsules seeder: