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
Fixed use of DRUPAL_ROOT in cache:rebuild command. #4713
Fixed use of DRUPAL_ROOT in cache:rebuild command. #4713
Conversation
Thanks for the PR. If the constant is unreliable, that’s a core bug and I'd rather not work around it in Drush. We use DRUPAL_ROOT elsewhere as well. |
Well in the relevant core issue, https://www.drupal.org/project/drupal/issues/1792310, there is discussion about deprecating DRUPAL_ROOT. So I think removing it from Drush would be a good idea. |
If that constant goes away, we'll use whatever its replacement is. |
@joachim-n Thank you for point me to this PR. Without this, when using the special structure of Even if this does get fixed in Drupal core, older Drupal versions (8.9.x, 9.1.x, 9.2.x) won't be able to take advantage of this fix. |
@shaal I think I've fixed the problem this PR had with newer versions of Drush. |
Ah nope, drush cr still kills off contrib modules. |
@weitzman I tried applying patch #5147 it doesn't solve the problem; it has the same issue as in @joachim-n's patch. I tested it locally (https://github.com/joachim-n/drupal-core-development-project + patch in this issue, as well as patch in #5147), and I tested it using DrupalPod. Steps to replicate:
|
Would be good if someone could see if #3391 is at fault. the symptom is the same. |
I don't think this is easily fixable in Drush.
This doesn't allow us to pass in anything about the app root, and so it will go to DrupalKernel::guessApplicationRoot(), which just works with DIR and so will get it wrong. The only thing to do would be for Drush to make a copy of drupal_rebuild() which passes in the correct app root when instantiating DrupalKernel. I doubt @weitzman would be happy with that! Although -- can a package alter an existing command to switch its callback? We could make a package which replaces the |
Thanks for the analysis. You are right that I don't relish maintaining a copy of drupal_rebuild(). I will however pass $root to it (see #5149) , and then interested folks can petition Drupal core to add an optional param to it. I don't see why anyone would object to that. That patch could be safely backported to 9.x and 8.x.
Yes, see https://github.com/consolidation/annotated-command#replace-command-hook. Perhaps https://github.com/consolidation/annotated-command#commandinfo-alterers would work as well. |
I don't think that's needed -- when the core DRUPAL_ROOT issue is fixed, this all should Just Work (famous last words!!) Though a fix to drupal_rebuild() might get in sooner, OTOH. |
Right. |
@shaal I've pushed a Drush command replacement to the composer project. Can you have a look? |
Code looks good. If you verified that your code is getting called instead drush's, then its good. If you have trouble with that, see https://www.drush.org/latest/commands/#auto-discovered-commands |
Thanks for having a look @weitzman. I've tried it on my local install, after a |
@joachim-n I tested your latest update in DrupalPod joachim-n/drupal-core-development-project@e3f5c7f It works! Thank you! |
Fixes #4712.