Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Add a return value to file_save_htaccess » Add a return value to file_save_htaccess()
Parent issue: » #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
FileSize
1.68 KB
+++ b/core/includes/file.inc
@@ -1495,8 +1500,10 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
+    // During early update there is no container. When called from
+    // UnitTestBase::setUp(), the container has no config.factory service.
+    $container = \Drupal::getContainer();
+    if (is_object($container) && $container->has('config.factory')) {

Removing this change, since unrelated.

sun’s picture

Issue tags: -Needs tests
FileSize
4.69 KB

Attached patch adds unit test coverage.

Note that the test fails on Windows due to #2107287: PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal

chx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: drupal8.file-htaccess-return.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
3.16 KB

Fixed File API is broken in DUTB tests + stream wrappers of parent site are leaking into all tests.

sun’s picture

Status: Needs review » Postponed

Recalled that I ran into the stream wrapper problem in tests a long time ago already.

Thus, extracted changes, and this is now blocked on #1376122: Stream wrappers of parent site are leaking into all tests

The last submitted patch, 5: file.htaccess.5.patch, failed testing.

sun’s picture

Two years later, #1376122: Stream wrappers of parent site are leaking into all tests is finally done and green now. :)

After that has landed, we can proceed here.

sun’s picture

Status: Postponed » Needs review
FileSize
5.25 KB
3.15 KB

The dependency is finally in... and it makes the new test here finally pass with HEAD as-is. :-)

Polished the new HtaccessUnitTest.

→ RTBC?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

alexpott’s picture

Title: Add a return value to file_save_htaccess() » Change notice: Add a return value to file_save_htaccess()
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record
alexpott’s picture

Committed 747fd61 and pushed to 8.x. Thanks!

sun’s picture

Thanks!

But hm. - A change notice for an added return value?

I'd consider that to be a bit too granular, no?

sun’s picture

Title: Change notice: Add a return value to file_save_htaccess() » Add a return value to file_save_htaccess()
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Closing this without a change notice for now, because I still think that an added return value is too minor to justify a change notice.

Please re-open if you disagree.

Status: Fixed » Closed (fixed)

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