Don't call bootstrapValidate commands repeatedly, and add --uri tests #3978

rmuit · 2019-03-09T21:47:17Z

These are two followups to #3847 which are not super related, but I hope putting them in one PR turns out to be less work.

I found out why bootstrapDrupalSiteValidate() is called a lot more lately (since Better bootstrap refactor #3820):

BootstrapManager::bootstrapMax() has a loop for each phase, in which it calls bootstrapValidate()
bootstrapValidate() only checks its $result_cache for the current target phase is filled, and if it is not, validates all phases from 0 up to this target phase again.

I'm going to just assume that was not intentional and that it can be fixed this way.

--

It was suggested to add a test, to add against regressions with the --uri flag.

So here it is. I'm guessing a fairly simple addition to CoreTest::testOptionsUri() will do the trick, even though that means it's now used to test two slightly different things.

Note - I just assumed that the 'expected' URLs are the ones that are returned by 'drush status' at this moment. I'm not 100% sure if that's true; you're in a better position to judge:

test.uri/ leads to outputting "http://test.uri/" for the Site URI. That wasn't the case before Better bootstrap refactor #3820 - i.e. in 9.6.0-beta3 (and all the way down to drush6), "test.uri/" was output instead.
I could imagine that http://test.uri and http://test.uri/ should actually both output the same result (i.e. without trailing slash) - but this was never the case before, so...

--

If this PR (containing both commits) is applied to the commit from before #3847, the test for "--uri=test.uri/subpath" fails as expected.

rmuit · 2019-03-09T21:49:55Z

tests/functional/CoreTest.php

+        fwrite($fp, '
+$sites["test.uri"] = "dev";
+$sites["test.uri.subpath"] = "stage";
+');


I'm not sure if I should have reverted sites.php after this test finishes... You're not using "test.uri" anywhere else apparently, because tests still succeed.

Shouldn't hurt to leave sites.php in place; you could even commit this to the git repository if you wanted. Optional.

weitzman · 2019-03-11T13:44:47Z

I'm not sure about the test (someone else should review it), but the bootstrapValidate() change looks right to me. I'll merge just that part, unless @greg-1-anderson chimes in otherwise.

greg-1-anderson · 2019-03-11T14:58:30Z

tests/functional/CoreTest.php

+        // Test whether a URI in a config file resolves correctly, and test
+        // various URI values for their expected Site URI and path.
        $drush_config_file = Path::join($this->webrootSlashDrush(), 'drush.yml');
-        $test_uri = 'http://test.uri';


We still want to test with --uri=http://test.uri. Looks like you removed this test.

Two answers, to make surer I'm not misinterpreting:

This test is still being done; it's just done as part of the loop.

This (the current/old test) isn't testing with --uri=http://test.uri; it's only writing "http://test.uri" to the config file and testing with --uri=OMIT on the commandline. It's slightly hard to see because of naming, that $options_with_uri is not the --uri option on the commandline. Which is also why I renamed the variables.

I could add a new drush invocation that tests --uri on the commandline (not writing config fille), though. Either with only one flag, or with all of them as part of the loop.

I'll have to re-read it later in context, but I think that $options_with_uri does have the --uri option on the commandline. At least that was my impression.

Ah, I see now. Tests look good.

greg-1-anderson

Basic changes to bootstrapping look right, but we want to continue to test with test.uri to make sure everything is still working. Make your sites.php test in addition to the tests that were there before. I don't think you even need to dynamically write sites.php; we could just add it to the sut and commit it, I think.

greg-1-anderson

I misread the code earlier. Tests look good. +1 on merging the whole thing once the tests come back green again.

rmuit · 2019-03-11T21:10:08Z

Pushed a 3rd commit moving the $sites configuration into sut/sites/example.sites.php as suggested... since that is the file which is copied.
Tests OK locally.

Commit can be deleted if it turns out that keeping example.sites.php pristine is a better idea.

greg-1-anderson · 2019-03-12T00:00:24Z

I forgot that we overwrite sites.php with example.sites.php repeatedly. Modifying example.sites.php would be okay, but Drupal Scaffold is going to overwrite this on composer update. We could either revert the last commit as suggested, or stop Drupal Scaffold from updating example.sites.php. I don't think we need an up-to-date version of this file for the sut.

greg-1-anderson · 2019-03-12T06:46:49Z

If we set extras.drupal-scaffold.excludes to sites/example.sites.php, then Drupal Scaffold would not overwrite the example.sites.php file.

@weitzman Do you have a preference about committing example.sites.php vs dynamically writing sites.php?

weitzman · 2019-03-12T13:42:51Z

I dont have a preference.

greg-1-anderson · 2019-03-12T15:11:01Z

I have a slight preference for committing static fixtures; I'll add the Drupal Scaffold exclude and merge in a little bit.

greg-1-anderson · 2019-03-12T15:43:27Z

We are not using Drupal Scaffold at the moment, so this is good to go.

rmuit changed the title ~~Undupe validate~~ Don't call bootstrapValidate commands repeatedly, and add --uri tests Mar 9, 2019

rmuit commented Mar 9, 2019

View reviewed changes

rmuit mentioned this pull request Mar 9, 2019

Continue #3166/2. Fix script name when Drupal uses a domain + subdir without scheme. #3847

Merged

greg-1-anderson reviewed Mar 11, 2019

View reviewed changes

greg-1-anderson requested changes Mar 11, 2019

View reviewed changes

greg-1-anderson approved these changes Mar 11, 2019

View reviewed changes

rmuit added 3 commits March 11, 2019 22:07

Don't call bootstrapValidate commands repeatedly

552bbb2

Add test for various forms of --uri options

aeb52ec

Move $sites configuration into example.sites.php.

ae8ba8e

rmuit force-pushed the undupe-validate branch from 73ebc2b to ae8ba8e Compare March 11, 2019 21:07

greg-1-anderson added this to the drush9 milestone Mar 12, 2019

Merge branch 'master' into undupe-validate

f7973ae

greg-1-anderson merged commit 70f75ea into drush-ops:master Mar 12, 2019

weitzman mentioned this pull request Apr 23, 2019

--uri requires trailing slash to work as expected #3911

Closed

Uh oh!

Don't call bootstrapValidate commands repeatedly, and add --uri tests #3978

Don't call bootstrapValidate commands repeatedly, and add --uri tests #3978

Uh oh!

Conversation

rmuit commented Mar 9, 2019

Uh oh!

rmuit Mar 9, 2019

Choose a reason for hiding this comment

Uh oh!

greg-1-anderson Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

weitzman commented Mar 11, 2019

Uh oh!

greg-1-anderson Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

rmuit Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

rmuit Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-1-anderson Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

greg-1-anderson Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

greg-1-anderson left a comment

Choose a reason for hiding this comment

Uh oh!

greg-1-anderson left a comment

Choose a reason for hiding this comment

Uh oh!

rmuit commented Mar 11, 2019

Uh oh!

greg-1-anderson commented Mar 12, 2019

Uh oh!

greg-1-anderson commented Mar 12, 2019

Uh oh!

weitzman commented Mar 12, 2019

Uh oh!

greg-1-anderson commented Mar 12, 2019

Uh oh!

greg-1-anderson commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmuit Mar 11, 2019 •

edited

Loading