Problem/Motivation

Based on this change record: https://www.drupal.org/node/2913019

...it is important that build tests use the .ht.router.php file provided by Drupal when they are serving requests via the PHP HTTP server.

The php built-in server has a small "bug" where it assumes all URLs w/ a period it them are for static assets. The .ht.router.php file overrides this functionality and looks for a resolvable route. If it finds one, it uses it instead of falling back on a static file. In the test case added here, .well-known/change-password is a route in user.module. And its pretty easy to see it fails.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

heddn’s picture

Automatic updates module uses a version strings (with periods in them) for its testing. Here's the diff of what I do over top of BTB to get those tests to pass.

diff --git a/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
index f698998f87..3b95aa78cd 100644
--- a/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -411,6 +411,9 @@ protected function instantiateServer($port, $working_dir = NULL) {
       '-t',
       $working_path,
     ];
+    if (file_exists($working_path . DIRECTORY_SEPARATOR . '.ht.router.php')) {
+      $server[] = $working_path . DIRECTORY_SEPARATOR . '.ht.router.php';
+    }
     $ps = new Process($server, $working_path);
     $ps->setIdleTimeout(30)
       ->setTimeout(30)
heddn’s picture

Status: Active » Needs review
FileSize
684 bytes

Let's start the discussion.

Mile23’s picture

This looks fine. We should also document in the docblock that .ht.router.php will be used if it's present.

heddn’s picture

FileSize
802 bytes
1.17 KB
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Excellent. :-)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Given 8.5.x is already not supported, and definitely not by the time 8.8.x ships, do we need the conditional?

Also, this may be a silly question, but is there anyway to test this? That's a pretty glaring bug to not use a file we're depending on, no?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

heddn’s picture

We can't always be sure how folks will use build tests. They might want to use it with 1) non-drupal install 2) very old drupal install 3) something else I can't think of where the router doesn't exist.

There is a way to test it and I can add a test.

heddn’s picture

FileSize
2.4 KB
4.66 KB

Interdiff is the tests only patch. Note, this pulls in quickstart base from #3082230: [meta] Convert some tests to Build tests. Which just means that meta will need a re-roll if it lands here first.

heddn’s picture

I really muffed up that test only patch. Here are updated patches.

The last submitted patch, 11: 3085728-tests_only.patch, failed testing. View results

heddn’s picture

Yeah, tests passed.

Mile23’s picture

Version: 8.9.x-dev » 8.8.x-dev

This counts as a test improvement for a new testing feature in 8.8.0, so we can target for 8.8.x.

heddn’s picture

Issue summary: View changes
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed f6b2481aee to 9.0.x and 2a58c7ef79 to 8.9.x. Thanks!

Will backport this to 8.8.x once the code freeze is over.

  • alexpott committed f6b2481 on 9.0.x
    Issue #3085728 by heddn, Mile23: Add access rules for build tests
    

  • alexpott committed 2a58c7e on 8.9.x
    Issue #3085728 by heddn, Mile23: Add access rules for build tests
    
    (...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch we agreed this was good to backport to 8.8.x

  • alexpott committed d947f1c on 8.8.x
    Issue #3085728 by heddn, Mile23: Add access rules for build tests
    
    (...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.