Skip to content
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

fix: drupal9 and drupal10 shouldn't be used with host-side drush so remove facility #5328

Merged
merged 1 commit into from Sep 12, 2023

Conversation

rfay
Copy link
Member

@rfay rfay commented Sep 11, 2023

The Issue

In Drupal7 and maybe early Drupal 8 it was common for people to run Drupal's drush (8) on the host side and so there was capability in the settings.ddev.php to allow this.

However, that's long since obsolete and just causes confusion now.

How This PR Solves The Issue

Remove the stanza from settings.ddev.php that set this up.

Manual Testing Instructions

Use DDEV with D9/D10. You should see a settings.ddev.php that doesn't have the stanza any more.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

@rfay rfay marked this pull request as ready for review September 11, 2023 22:52
@rfay rfay requested a review from a team as a code owner September 11, 2023 22:52
@simesy
Copy link

simesy commented Sep 12, 2023

I agree that using Drush from the host is not best practice and this is a good PR.

@rfay rfay merged commit 73c0e73 into ddev:master Sep 12, 2023
24 checks passed
@rfay rfay deleted the 20230911_remove_drush_d9_d10 branch September 12, 2023 01:36
@bohemier
Copy link

bohemier commented Nov 3, 2023

Just wondering... why is this not good practice? Just found out this issue after upgrading ddev to 1.22 as to why our drush scripts aren't working anymore... We were quite happy with using local drush with D9 / D10. It's actually a lot faster than using ddev drush.

time drush cr
[success] Cache rebuild complete.
drush cr  0.88s user 0.27s system 39% cpu 2.881 total

time ddev drush cr
[success] Cache rebuild complete.
ddev drush cr  0.09s user 0.10s system 1% cpu 10.888 total

Is there any way to bring this back?

Thanks :)

@rfay
Copy link
Member Author

rfay commented Nov 3, 2023

Oh, you want to use host-side drush still?

All you have to do is add the removed stanza here to your settings.php or settings.local.php.

When you use drush on the host side, you're depending on the host's version of php, and the host's version of drush, and it's just not a sustainable arrangement. It can cause lots of "interesting" behavior. In general, we encourage people to do everything repeatably inside the container, where the configuration is constrained. When you do this using host-side drush, every developer's experience can be different.

But you can certainly just add this stanza to your settings.php/settings.local.php and be on the way!

@bohemier
Copy link

bohemier commented Nov 3, 2023

Thanks for your reply Rfay!

In my experience, the host drush version matters little since it is only used to find the proper drush (most likely within /vendor/bin) and call it. So you always run the same version of drush in a composer-based setup. The only difference is what is used to launch drush.

As far as php version goes... yes this is something the developer has to manage, i.e. matching it with the web container's php version. This is usually not a big issue... and can be solved with brew-php-switcher as needed. A tradeoff we are happy to make to save 8 seconds each time the cache is cleared!

I was thinking of adding the code snippet as you suggest, but it seems actually a little tricky since it depends on the local port of the db container, and given this is adjusted dynamically by ddev:

// If DDEV_PHP_VERSION is not set but IS_DDEV_PROJECT *is*, it means we're running (drush) on the host,
// so use the host-side bind port on docker IP
if (empty(getenv('DDEV_PHP_VERSION') && getenv('IS_DDEV_PROJECT') == 'true')) {
  $host = "127.0.0.1";
  $port = 49589;
}

From what I have observed, $port varies every time we start ddev... Do you think we would have to write a post-start command that would query ddev about the local port for the db container, then place this in settings.local.php?

Hope you can suggest an easier fix :)
Thanks

@rfay
Copy link
Member Author

rfay commented Nov 3, 2023

For the port, use $DDEV_HOST_DB_PORT and it should be good. So something like

if (empty(getenv('DDEV_PHP_VERSION') && getenv('IS_DDEV_PROJECT') == 'true')) {
  $host = "127.0.0.1";
  $port = getenv("DDEV_HOST_DB_PORT");
}

Unfortunately, I'm not sure your drush on the host side will know about that.

I don't understand why you have performance problems when running drush inside the container.

If you ddev ssh and run drush cr do you see similar results? It doesn't actually make sense that the action from the host would be faster, as it has to use the exact same database action. But maybe your drush cr acts on files as well? What OS and Docker provider are you normally using? Is mutagen enabled?

@bohemier
Copy link

bohemier commented Nov 3, 2023

Thanks rfay,

I am unable to locate the environment variable DDEV_HOST_DB_PORT on either the host or any of the web or database containers.

However, I was able to get something working by creating a script that goes like this:

DDEV_DB_PORT=$(ddev status --json-output | jq '.raw.dbinfo.published_port')
TOP_LEVEL=$(git rev-parse --show-toplevel)
DDEV_SETTINGS_FILE=$TOP_LEVEL/docroot/sites/default/settings.ddev.php
DDEV_MAGIC_STANZA="\
// Replace the begone magic stanza, see https://github.com/ddev/ddev/pull/5328    \n\
if (empty(getenv('DDEV_PHP_VERSION') && getenv('IS_DDEV_PROJECT') == 'true')) {   \n\
  \$host = \"127.0.0.1\";                                                         \n\
  \$port = $DDEV_DB_PORT;                                                         \n\
}\n"

pos=$(grep -n '$driver = "mysql";' $DDEV_SETTINGS_FILE | cut -d: -f1)
if [ ! -z "$pos" ]; then
  sed -i "${pos}a $DDEV_MAGIC_STANZA" $DDEV_SETTINGS_FILE
fi

I feel this is not as robust as the original ddev functionality and it could probably be improved...

You are right, mutagten speeds up the process. It is, however, very time-consuming to start on some projects. My tests indicate that enabling mutagen and opening a second shell with ddev ssh yields the same time results as running on the host.

Still... we liked the simplicity of running drush from the host ;)

Thanks for this great project, you and the ddev team really earned the 2023 Aaron Winborn Award!

@rfay
Copy link
Member Author

rfay commented Nov 3, 2023

If you're having trouble starting up mutagen, it means that upload_dirs needs to be configured. A normal project will normally take only 5-10 seconds after the first start, and will take far less than a minute the first time. I'll be happy to take a look with you, but you may want to look at https://ddev.readthedocs.io/en/latest/users/install/performance/, or if you're using Docker Desktop, try using Orbstack, which is vastly faster than Docker Desktop.

@bohemier
Copy link

Thanks for all your help rfay... We are trying to patch things around to fix the departure of this feature, but I really wish this feature could be brought back into ddev if possible, as we also have issues with our git hooks no longer working. Was this feature causing problems elsewhere?

@rfay
Copy link
Member Author

rfay commented Nov 10, 2023

we also have issues with our git hooks no longer working. Was this feature causing problems elsewhere

Please open an issue with the specifics of your git hook problem. I don't know how it could have to do with DDEV. Please say in the issue whether you're using git inside the container or on the host.

As mentioned above, you don't need DDEV to add this feature, you can just set it up in your settings.php.

@rfay
Copy link
Member Author

rfay commented Nov 11, 2023

@bohemier

Could you please try out this new add-on?

ddev get rfay/ddev-drushonhost

And let me know how it goes?

https://github.com/rfay/ddev-drushonhost

@selwynpolit
Copy link

I tried it and it works great. Thanks @rfay

@bohemier
Copy link

@rfay this works very well and brings back the feature properly! Many thanks!

@rfay
Copy link
Member Author

rfay commented Nov 15, 2023

I still would love to understand why there's a performance difference. Followups in

please. I would love to know the answer. The only thing I can think is if drush is doing extensive things to sites/default/files. That could be tested by turning off the bind-mount of sites/default/files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants