Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load the stream handler class to register the stream wrapper #18060

Closed
wants to merge 1 commit into from

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Sep 22, 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.

@Bakual
Copy link
Contributor

Bakual commented Sep 22, 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.

@mbabker
Copy link
Contributor

mbabker commented Sep 22, 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.

@Bakual
Copy link
Contributor

Bakual commented Sep 22, 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.

@mbabker
Copy link
Contributor

mbabker commented Sep 22, 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.

@laoneo
Copy link
Member Author

laoneo commented Sep 22, 2017

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

@Bakual
Copy link
Contributor

Bakual commented Sep 22, 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

@laoneo
Copy link
Member Author

laoneo commented Sep 23, 2017

Closing in favor of #18075.

@laoneo laoneo closed this Sep 23, 2017
@laoneo laoneo deleted the j3/fix/stream/handler branch September 23, 2017 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants