Problem/Motivation

LocalReadOnlyStream is extending the LocalStream. LocalStream implements ::getType() and returns StreamWrapperInterface::NORMAL. But that composite bit flag pack signals that the stream wrapper is writable. So LocalReadOnlyStream says that is writable.

Proposed resolution

Implement ::getType() in LocalReadOnlyStream and signal a read-only stream wrapper.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

claudiu.cristea created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, read_only_stream_test_only.patch, failed testing.

claudiu.cristea’s picture

Title: LocalReadOnlyStream says that is writable » LocalReadOnlyStream says that he's writable
Issue summary: View changes
claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, read_only_stream_test_only.patch, failed testing.

klausi’s picture

Title: LocalReadOnlyStream says that he's writable » LocalReadOnlyStream must declare itself not writeable

Makes sense!

Fixing title for gender neutrality.

  1. +++ b/core/modules/file/tests/src/Kernel/LocalReadOnlyStreamTest.php
    @@ -0,0 +1,25 @@
    +class LocalReadOnlyStreamTest extends KernelTestBase {
    

    why is this a kernel test? No need for a database here, so this can be a unit test, right?

  2. +++ b/core/modules/file/tests/src/Kernel/LocalReadOnlyStreamTest.php
    @@ -0,0 +1,25 @@
    +    $this->assertFalse($type & StreamWrapperInterface::WRITE);
    

    assertFalse() is not correct here, since the result of the bitwise AND will be another integer (type mismatch). So this should be assertSame(0, $type & StreamWrapperInterface::WRITE)

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new952 bytes
new1.56 KB
new1.3 KB

@klausi, thank you

The last submitted patch, 7: localreadonlystream-2767269-7-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: localreadonlystream-2767269-7.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: localreadonlystream-2767269-7.patch, failed testing.

The last submitted patch, 7: localreadonlystream-2767269-7-test-only.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new1.56 KB

I don't understand this CI error :(

Status: Needs review » Needs work

The last submitted patch, 13: localreadonlystream-2767269-7.patch, failed testing.

Mixologic’s picture

Sorry I didnt see the ask in #drupal-contribute, but I ran the testbot locally, and its a pretty classic error. The testbots should be better about this kind of reporting:

Available test groups & classes
-------------------------------

exception 'Drupal\simpletest\Exception\MissingGroupException' with message 'Missing @group annotation in Drupal\Tests\Core\File\LocalReadOnlyStreamTest' in /var/www/html/core/modules/simpletest/src/TestDiscovery.php:351
Stack trace:
#0 /var/www/html/core/modules/simpletest/src/TestDiscovery.php(177): Drupal\simpletest\TestDiscovery::getTestInfo('Drupal\\Tests\\Co...', '/**\n * @coversD...')#1 /var/www/html/core/modules/simpletest/simpletest.module(571): Drupal\simpletest\TestDiscovery->getTestClasses(NULL, Array)
#2 /var/www/html/core/scripts/run-tests.sh(69): simpletest_test_get_all(NULL)
#3 {main}
claudiu.cristea’s picture

StatusFileSize
new946 bytes
new498 bytes

@Mixologic,

Ah, right, stupid me. PhpStorm has auto created @package instead of @group and I didn't check twice.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: localreadonlystream-2767269-16-test-only.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new1.55 KB

In #16 we have the prove. Now, the patch.

Status: Needs review » Needs work

The last submitted patch, 19: localreadonlystream-2767269-20.patch, failed testing.

claudiu.cristea’s picture

What?

The last submitted patch, 19: localreadonlystream-2767269-20.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

The failure is not related to this patch. See #2749955: Random fails in UpdatePathTestBase tests.

amateescu’s picture

+++ b/core/tests/Drupal/Tests/Core/File/LocalReadOnlyStreamTest.php
@@ -0,0 +1,24 @@
+    $type = DummyReadOnlyStreamWrapper::getType();
+    // Checks that the stream wrapper type is not declared as writable.
+    $this->assertSame(0, $type & StreamWrapperInterface::WRITE);

Can't we add this assertion to the existing \Drupal\KernelTests\Core\File\ReadOnlyStreamWrapperTest::testReadOnlyBehavior() instead of creating a whole new test class just for a method?

claudiu.cristea’s picture

StatusFileSize
new1.67 KB
new1.95 KB
new1.65 KB

@amateescu, yes, that makes a lot of sense.

In 8.1.x ReadOnlyStreamWrapperTest was an old-style kernel test and was placed inside system.module, thus I provided also a patch for 8.2.x.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Awesome :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: localreadonlystream-2767269-25.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7b0d229 and pushed to 8.2.x. Thanks!
Committed 48b3d70 and pushed to 8.1.x. Thanks!

  • alexpott committed 7b0d229 on 8.2.x
    Issue #2767269 by claudiu.cristea: LocalReadOnlyStream must declare...

  • alexpott committed 48b3d70 on 8.1.x
    Issue #2767269 by claudiu.cristea: LocalReadOnlyStream must declare...
claudiu.cristea’s picture

Unfortunately I forgot to add also the StreamWrapperInterface::LOCAL bit to the result of LocalReadOnlyStream::getType(). I was wrong, thinking that READ_VISIBLE composite value contains also LOCAL without testing that. I opened a followup in #2770339: LocalReadOnlyStream must declare itself LOCAL.

  • alexpott committed 7b0d229 on 8.3.x
    Issue #2767269 by claudiu.cristea: LocalReadOnlyStream must declare...

  • alexpott committed 7b0d229 on 8.3.x
    Issue #2767269 by claudiu.cristea: LocalReadOnlyStream must declare...

Status: Fixed » Closed (fixed)

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