protected function registerStreamWrapper($scheme, $class, $type = STREAM_WRAPPERS_LOCAL_NORMAL) {
    if (isset($this->streamWrappers[$scheme])) {
      $this->unregisterStreamWrapper($scheme);
    }

My IDE says: "Required parameter $type missing" on the call to unregisterStreamWrapper().

CommentFileSizeAuthor
#9 2279213-9.patch724 bytesneclimdul
#1 drupal8.ktb-stream-unregister.1.patch724 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.ktb-stream-unregister.1.patch724 bytes
donquixote’s picture

donquixote CreditAttribution: donquixote commented

Unbelievable, I just made the very same patch, but was too late :)

See also
#2028109: Convert hook_stream_wrappers() to tagged services.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Being as this isn't throwing any errors, is it possible this is just a dead code path that should be removed? Alternatively and probably more likely, should we provide test coverage for this code path?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

It's an extra safety net for convenience. The code path is not triggered in HEAD, because to my knowledge, there is no test that tries to replace an existing stream wrapper.

So alternatively, we could also remove that condition. Net effect is a PHP Warning/Error in tests that try to replace an existing stream wrapper without unregistering the existing first.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

If you register a new stream wrapper with the same name, is it intended to remove the old?
Or should we rather throw an exception, assuming that replacing a stream wrapper unintentionally could cause trouble elsewhere?

Imo, let's apply a minimal fix here, and do the rest in #2028109: Convert hook_stream_wrappers() to tagged services..
(see the @todo in unregisterStreamWrapper())

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

As mentioned in #4, PHP core throws warning/error if you try to register a scheme that is registered already.

Hence, manually throwing an exception is unnecessary, because your test will fail either way.

sun queued 1: drupal8.ktb-stream-unregister.1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal8.ktb-stream-unregister.1.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review
FileSize
2279213-9.patch724 bytes

Wow, this got lost... Lets just go with the fix for now. I don't know what is really right and I'm guessing by the lack of motion that's sort of the consensus. This at least fixes the error in the code so it will work as originally intended.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: KernelTestBase::unregisterStreamWrapper() requires two arguments. » KernelTestBase::unregisterStreamWrapper() is called once with insufficient arguments
Status: Needs review » Reviewed & tested by the community

Yup, let's simply forward here please. Thanks for re-rolling!

My ultimate hope is that most of this funky code can hopefully be replaced by #2028109: Convert hook_stream_wrappers() to tagged services. — that's a very long shot from status quo, but one is allowed to have a vision, right?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2279213-9.patch, failed testing.

Status: Needs work » Needs review

neclimdul queued 9: 2279213-9.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2279213-9.patch, failed testing.

Status: Needs work » Needs review

sun queued 9: 2279213-9.patch for re-testing.

Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 268435456 bytes)
  in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php on line 336

Drupal\system\Tests\Database\SelectTableSortDefaultTest       30 passes
FATAL Drupal\system\Tests\Entity\EntityCrudHookTest: test runner returned a non-zero error code (255).

Sent Client #2498 for client re-test.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 351352e and pushed to 8.x. Thanks!

  • alexpott committed 351352e on 8.x 
    Issue #2279213 by neclimdul, sun | donquixote: Fixed KernelTestBase::...

Status: Fixed » Closed (fixed)

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