Project

General

Profile

Actions

Bug #94422

closed

wrong handling in TYPO3\CMS\Core\Http\SelfEmittableLazyOpenStream

Added by David Bruchmann almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Install Tool
Target version:
-
Start date:
2021-06-27
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
11
PHP Version:
7.4
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

The class SelfEmittableLazyOpenStream has one issue like reported in the comments here https://forge.typo3.org/issues/91826 but furthermore another one.
In bug 91826 I mention the context and won't repeat it here.

Here is the additional bug:

The method write($string) is not according to the interface Psr\Http\Message\StreamInterface and must return int.
Therefore it must not throw any exception. As the class is used for Ajax requests exceptions are not shown and make debugging quite hard anyway.
Having tried to return just 0 I never got the installtool running like expected.
Returning just strlen($string) instead worked so far that the install-process finished by writing in LocalConfiguration.php and the backend could be opened.

I don't know which string is coming in, if it could include multi-byte-signs and therefore return a wrong strlen.
Also I never verified the usage of the returned value.
Therefore my solution is a workaround for my own case but it has to be verified if it could be used in general.


Related issues 3 (1 open2 closed)

Related to TYPO3 Core - Bug #91826: Install issue in TYPO3 10/11New2020-07-19

Actions
Related to TYPO3 Core - Bug #94454: SelfEmittableLazyOpenStream is not compatible with guzzlehttp/psr7 2.0.0Closed2021-07-01

Actions
Related to TYPO3 Core - Task #94800: Update codeception dependenciesClosedBenni Mack2021-08-11

Actions
Actions #1

Updated by David Bruchmann almost 3 years ago

  • Related to Bug #91826: Install issue in TYPO3 10/11 added
Actions #2

Updated by David Bruchmann almost 3 years ago

The Exception in the method write($string) seems being correct according to the StreamInterface, nevertheless it had to be thrown conditional and not always.

Here is the method in the StreamInterface:

    /**
     * Write data to the stream.
     *
     * @param string $string The string that is to be written.
     * @return int Returns the number of bytes written to the stream.
     * @throws \RuntimeException on failure.
     */
    public function write($string);

and below is shown how it's implemented in TYPO3:

    /**
     * @param string $string
     * @throws \RuntimeException on failure.
     */
    public function write($string)
    {
        throw new \RuntimeException('Cannot write to a ' . self::class, 1538331833);
    }

Following is my workaround to get it running:

    /**
     * @param string $string
     * @return int Returns the number of bytes written to the stream.
     * @throws \RuntimeException on failure.
     */
    public function write($string) : int
    {
        // throw new \RuntimeException('Cannot write to a ' . self::class, 1538331833);
        return strlen($string);
    }

Actions #3

Updated by David Bruchmann almost 3 years ago

Having discovered the trait https://github.com/guzzle/psr7/blob/148492f9e2e1524317a8aa60875f5b59517a9374/src/StreamDecoratorTrait.php which I did miss before, I see that the implementation in TYPO3 just by extending GuzzleHttp\Psr7\LazyOpenStream might be wrong.

The class LazyOpenStream serves as stream-wrapper and just returns what a stream-class is returning. Therefore any streams should be transferred as argument and not by extending the class.

Actions #4

Updated by Christian Kuhn almost 3 years ago

Ok, guzzle/psr7 2.0-dev declares LazyOpenStream final. That's why it fails - we can't extend that class anymore.

This did not pop up in core testing yet, since we don't install -dev dependencies in our 'composer max' suites, and you probably shouldn't, either. Please stick to non-dev dependencies, core gives no guarantees whatsoever on this.

Of course, we should still change our SelfEmittableLazyOpenStream to deal with this dependency change, but it's not a blocker (currently, at least until 2.0.0 is out). For v10, we might be better off with defining guzzle/psr7 2.0 as conflict and adapt only v11 to be later compatible with guzzle/psr7 when 2.0.0 is released.

Actions #5

Updated by Christian Kuhn almost 3 years ago

Easiest solution for both v10 and v11:

v10:
composer --req guzzle/psr7:^1.4.0

v11:
composer --req guzzle/psr7:^1.7.0

Both are the current lower bounds given by current v10 / v11 guzzlehttp/guzzle dependency.

This will deny 2.0 and also declares that we actively use guzzle/psr7 as dependency, which we do, since we extend one of its classes.

Agreed?

Actions #6

Updated by Christian Kuhn almost 3 years ago

Above notes are more for the 'final' report from https://forge.typo3.org/issues/91826#note-3 - still unsure if there is more todo. I'd say we should block 2.0 for now, then see what happens. ok?

Actions #7

Updated by David Bruchmann almost 3 years ago

Sounds reasonable to block v2.0 first as quick fix.
I think an update should be target in mid-term though.
I haven't verified the differences between the versions so I don't know if v1.x has really disadvanteges.
Christian, you described it more detailed how to handle it, that's completely fine I think.

Actions #8

Updated by Gerrit Code Review almost 3 years ago

  • Status changed from New to Under Review

Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/69666

Actions #9

Updated by Gerrit Code Review almost 3 years ago

Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/69667

Actions #10

Updated by Gerrit Code Review almost 3 years ago

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/69668

Actions #11

Updated by Christian Kuhn almost 3 years ago

Pushed patches for master, v10 and even v9 since a guzzlehttp/psr7 2.0.0 would break v9 otherwise.

David, would you like to test that? It should actively downgrade your guzzlehttp/psr7 to 1.something, probably 1.8.2.

Actions #12

Updated by Christian Kuhn almost 3 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #13

Updated by Christian Kuhn almost 3 years ago

  • Related to Bug #94454: SelfEmittableLazyOpenStream is not compatible with guzzlehttp/psr7 2.0.0 added
Actions #14

Updated by Christian Kuhn over 2 years ago

  • Related to Task #94800: Update codeception dependencies added
Actions #15

Updated by David Bruchmann over 2 years ago

The method write($string) still has to be changed.
Btw. according to the interface it must throw an exception but return int if I remember correctly.

Actions #16

Updated by Benni Mack over 2 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF