- Issue created by @dpi
- Status changed to Needs review
7 months ago 2:38pm 9 June 2024 - 🇦🇺Australia dpi Perth, Australia
The intricacies of how to match versions in Composer with Drupal build system needs work, otherwise seeking feedback on the IS/proposal and the non Composer/build-system related pieces of the MR.
- 🇷🇺Russia Chi
+1
I've built a POC a few years ago. That proves how easy to create a CLI endpoint for commands that only working on fully booted Drupal site. That's an example of Pareto principal. Just a few commands (like
drush site-install
) that need to support non-booted environment, brought so much complexity to Drush.One thing I'd like see in this implementation is supporting command discovery in vendor directory. So you would be able bundle console commands to a Composer package (not Drupal module).
- 🇦🇺Australia dpi Perth, Australia
That's an example of Pareto principal. Just a few commands
This is exactly what I was thinking. Well put.
One thing I'd like see in this implementation is supporting command discovery in vendor directory. So you would be able bundle console commands to a Composer package (not Drupal module).
I've tried to address this in MR Notes ✨ CLI entry point in Drupal Core (Dex) Needs review . You can absolutely set up a command that exists in vendor (via services yml, etc). However autodiscovery is limited to the same rules as Drupal, i.e. only in module directories. Until Drupal has the ability to discover modules/plugins/etc I dont think it makes sense for this issue/initiate or related issues like ✨ Directory based automatic service creation Needs review to go out of their way and look in vendor. Its a large can of worms that doesnt need a decision now; we can easily change it in the future.
I've built a POC a few years ago.
Neat, I'm glad we've evolved even further since then. We're in such a nice spot in 2024.
- 🇫🇷France andypost
+1 sounds like solution to long lasting set of issues, it could be extendable via
dex drush
PS looking how to extend it for xhprof but SF runtime backend is one more strong reason
- Status changed to Needs work
6 months ago 1:47pm 11 June 2024 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States greg.1.anderson
Overall, I like the proposal. Drush can already accept Symfony Console commands in its command list, so if the command loader was available as a public method of a discoverable class, then all dex-provided commands from modules could also be available via
drush commandname
, for existing Drush users, as desired.Regarding the Drupal core cli, though, I think that the command name should remain
drupal
, and the work here should be limited to providing a mechanism whereby modules can add new commands to this existing script. - 🇺🇸United States mradcliffe USA
I think "Anything supporting a non-booted site." should be in-scope so that we can implement this as part of the existing drupal core CLI. Having an intermediary step where we have dex and drupal and then switch back to drupal later on would seem like a lot of wasted time updating docs, training materials, tests, etc..
If we can have a resolution for both existing sites and new sites, then it would be easier to adopt to using this rather than a new command.
- 🇷🇺Russia Chi
How many commands in Drush core can work on non-booted Drupal installation?
I found just three.
- site:install
- archive:dump
- archive:restore
I believe those commands deserve own console endpoint.
- 🇺🇸United States greg.1.anderson
The main use-case for non-bootstrapped commands is the existing
drupal install
. - 🇦🇺Australia acbramley
I think "Anything supporting a non-booted site." should be in-scope
I think the IS and other comments here make a compelling argument against this.
- 🇺🇸United States mradcliffe USA
@acbramley, I disagree based on my previous comment about multiple commands being more confusing so solving the problem of bootstrap/non-bootstrap is important now rather than later.
- 🇺🇸United States greg.1.anderson
The issue summary says:
Perhaps a future all-encompassing Drush replacement would utilise the drupal namespace and cleanly replace dex.
It is not necessary to build an entire Drush replacement to unify the
dex
anddrupal
front controllers. We have a paved path for doing this in Drush that wouldn't be too difficult to use here. It's harder to remove things that you don't want than it is to add something new, so I do not find it to be a compelling argument that we should introduce a new script only to immediately deprecate it.I think we could get this merged if we unified on the
drupal
script name. I personally feel like we have too many different command names that are trying to do the same thing, and expanding that collection further is not moving in the right direction. - 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
For commands like
site:instlall
, what if those were just Compsoer plugins? We already recommend thecomposer create-project
command as the first command you run, what if the next command was justcomposer drupal-install
.Then you have a installed site, and move onto using
drupal
(or whatever the name is).For the record, I prefer just
drupal
as the command name.I don't know too much about the existing
drupal
command, but it seems to not require a booted site, so if the non-booted parts of that were just moved to Composer commands, that would free up thedrupal
command to become one which can be used for a booted site.drupal
atcore/scripts
could then be deprecated, and eventually replaced, whiledrupal
atcore/bin
is what we are building here. - 🇺🇸United States greg.1.anderson
I can't help but feel like some of the strong opinions against combining
dex
anddrupal
might be motivated by an impression that taking that step would pull in a lot of complexity from the Drush bootstrap logic in order to support both bootstrapped and non-bootstrapped commands. In actuality, though, this can be achieved with a little over a dozen lines of code by overriding the Application::find() method. Drush has been doing this successfully for years, since Drush 9, and it has worked well across multiple versions of Symfony. The rest of Drush's preflight and bootstrap can be left behind. The key part look like this (in the derived Application class):public function find($name): Command { try { $command = parent::find($name); if ($command instanceof ListCommand || $command instanceof HelpCommand) { $this->bootstrap(); } return $command; } catch (CommandNotFoundException $e) { if (!$this->bootstrap()) { throw $e; } return parent::find($name); } }
I have created a branch that demonstrates the above technique in action. Bootstrapped and non-bootstrapped commands live side-by-side in the
drupal
script. This fulfills the vision from the IS: to eventually replacedex
with a unifieddrupal
command. The rest of the code in the branch needed to make this work, beyond the fourteen lines shown above, is pretty much the same code that is in the current MR, with some refactoring. I have left the originaldex
script in the branch, so that it can be tried alongside the enhanceddrupal
script. Some enhancements fromdex
, e.g. the wrapper invendor/bin
, have been carried over from the original MR and applied to thedrupal
script.I think that the work done in the original MR here is really excellent. The advent of attribute-based discovery, and conformance to the latest Symfony conventions around command definition gives us a really clean interface to allow Drupal modules to add cli commands to a lightweight front controller in core. I think that we have a really good chance at getting this merged, if the community can reach an agreement on whether the script that is used to call Drupal module commands should be called
dex
ordrupal
. I hope that the minimum additions I've shown here are sufficient to convince folks that we should integrate with the existingdrupal
script now rather than later. - 🇮🇹Italy kopeboy Milan
Another reason not to use Dex is it is a very common acronym in the web3 (blockchain) space (meaning Decentralized Exchange).
- First commit to issue fork.