User tests: Successful: Unsuccessful:

avatar laoneo
22 Sep 2017

Pull Request for Issue #18024.

Summary of Changes

The stream handler needs to be registered correctly in the FTP client.

Testing Instructions

See #18024.

Expected result

It works.

Actual result

A buffer not exists error is thrown.

avatar joomla-cms-bot joomla-cms-bot - change - 22 Sep 2017
Category Libraries
avatar laoneo laoneo - open - 22 Sep 2017
avatar laoneo laoneo - change - 22 Sep 2017
Status New Pending
avatar Bakual Bakual - test_item - 22 Sep 2017 - Tested successfully
avatar Bakual
Bakual - comment - 22 Sep 2017

I have tested this item successfully on 10b7106

Yes, this fixes the issue for me.

It's a hack however, but I didn't have a better idea.

I think the real issue is in the BufferStreamHandler class. That the registering of the buffer happens outside the class. (https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Utility/BufferStreamHandler.php#L14)
But that can't be changed within 3.x I think, it certainly should for 4.0 imho.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18060.

avatar mbabker
mbabker - comment - 22 Sep 2017

It's not even that. Even if the stream_wrapper_register() call was in the buffer class' constructor, something would still have to load the file and class for it to work. So really the call almost needs to be in the application bootstrap (libraries/bootstrap.php in 4.0, the plethora of files doing it all in 3.x), but then we also need to make sure that arbitrarily setting the stream handler like that doesn't come with its own set of side effects.

You're right in that it's not an elegant solution, but it is a valid one for now.

avatar Bakual
Bakual - comment - 22 Sep 2017

something would still have to load the file and class for it to work

Yeah, I was thinking along the way of adding a method "stream_register" to the class in J4.0 which checks if the stream is already registered and registers it if not already done. And yes, each class using that stream would have to instantiate the class and call the method. From what I see, it's only used in the FtpClient class anyway (in core).
If it's fine to move it to the bootstrapping file, that of course would be simpler.

avatar mbabker
mbabker - comment - 22 Sep 2017

That works too. And could be done in 3.x. Actually, should be done because the existing "trick" used in 3.7 and earlier has broken.

In 3.x we add the call to register the stream arbitrarily into our bootstrap then in 4.0 we can have the minor B/C break of instructing developers who need it to explicitly call that register method.

avatar laoneo
laoneo - comment - 22 Sep 2017

If we do it in bootstrap then it is loaded every time, do we want that?

avatar Bakual
Bakual - comment - 22 Sep 2017

I've written a PR based on the last comments. I think it can be done B/C without putting it into the bootstrapping file. See #18075

avatar laoneo
laoneo - comment - 23 Sep 2017

Closing in favor of #18075.

avatar laoneo laoneo - change - 23 Sep 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-09-23 07:46:56
Closed_By laoneo
Labels Added: ?
avatar laoneo laoneo - close - 23 Sep 2017

Add a Comment

Login with GitHub to post a comment