[RFC] [Discussion] Add openStream() to XML{Reader,Writer}

Le 23 avr. 2024 à 21:23, Niels Dossche dossche.niels@gmail.com a écrit :

Hi internals

I'm opening the discussion for my RFC "Add openStream() to XML{Reader,Writer}".
RFC link: https://wiki.php.net/rfc/xmlreader_writer_streams

Kind regards
Niels

This seems quite reasonable to me overall. My one question is regarding the writer version. Why is that not a static method, too? I would have expected that to be a "named constructor" just like the reader.

--Larry Garfield

Hi Larry

XMLReader already had these static methods that act as named constructors, but XMLWriter has no named constructors at the moment.
The XMLWriter::openMemory() and XMLWriter::openUri() functions are instance methods that must be called after doing "new XMLWriter".
If these two existing functions were static methods instead, I would've made XMLWriter::openStream() static too.
So IOW, for consistency I followed the model of the existing XMLWriter methods.

While it is possible to do the magic trick that XMLReader uses to have the open methods on XMLWriter both static and non-static, this is quite hacky and was only done to XMLReader for BC reasons.

That’s odd. The inconsistency was introduced (or at least sanctioned) in PHP 8.0. In PHP 7, XMLReader::open() and XMLReader::XML() already worked when used both as static and non-static methods, but triggered a deprecation warning when called statically. The deprecation warning was removed in 8.0, regardless of the differing semantics when called statically and non-statically, and regardless of the the inconsistency with corresponding XMLWriter methods.

Another point: when called statically on a subclass, both XMLReader::open() and XMLReader::XML() return an object of type XMLReader, not of the subclass: https://3v4l.org/lCOAvJ
For that reason, they are unusable as static methods on a subclass. The new openStream() method should work on instances, so that it will be usable on subclasses. (And for the same reason, I think it was a mistake to undeprecate XMLReader::{open,XML}() as static methods in 8.0.)

—Claude

Hi internals

I'm opening the discussion for my RFC "Add openStream() to XML{Reader,Writer}".
RFC link: https://wiki.php.net/rfc/xmlreader_writer_streams

Kind regards
Niels

Hi internals

It's been over two weeks since I opened the discussion.
Please raise any last concerns now.
I'd like to start voting next week Monday.

Kind regards
Niels

Please include some examples of what a full usage would look like with the new additions. I'm not super experienced with those libraries right now, but the "create the object, THEN populate it with its data source" pattern feels very weird to me. (Though at least it's consistent now rather than as it was before, thanks.)

--Larry Garfield

Hi Larry

I added minimal examples for both.
The streams I used are just the memory and output streams for simplicity so that it can be tested on its own.

Kind regards
Niels

On Tue, May 21, 2024 at 12:58 PM Claude Pache claude.pache@gmail.com
wrote:

Le 18 mai 2024 à 01:13, Niels Dossche dossche.niels@gmail.com a écrit
:

Hi internals

I'm opening the discussion for my RFC "Add openStream() to
XML{Reader,Writer}".
RFC link: https://wiki.php.net/rfc/xmlreader_writer_streams

Kind regards
Niels

Hi internals

The main complaint that kept coming up in internal communication was the
choice of instance methods instead of static methods.
This has been brought up in this thread too.

Hi,

Now you have a complaint because of the choice of static methods instead
of instance methods... :-)

You are introducing an inconstancy for subclasses, because:

• As noted in an earlier message, the existing open-like methods
  (XMLReader::XML() and XMLReader::open()) don’t work statically on
  subclasses (they create an instance of the base class, not of the
  subclass). You must use them as instance methods.

• On the other hand, the method you are going to introduce won’t work as
  instance method. You must use it statically.

Please, if you want to make the new method static only, fix first the
existing ones, so that they also work statically (on subclasses).

(But again, I prefer that all those methods work on instances, as it was
the case before PHP 8. They shouldn’t have been switched to
static-but-broken-for-subclasses without discussion.)

Fixing the existing ones would be a potential BC break, depending on
whether or not instance usage is completely eliminated.

The reason why many of us recommended static methods here is that they
would act as named constructors, and reduce the potential for cases where a
new stream is read from or written to mid-flight. Essentially, the usage
becomes:

$reader = XMLReader::fromStream($streamHandle);
 // read the stream

and

$writer = XMLWriter::toStream($streamHandle);
// write to the stream

My understanding is that the implementations will allow for subclassing.

Having these as instance methods means that subclasses would need to
potentially add handling to ensure the instance is not in an invalid state
(e.g., by switching streams mid-flight), which would add far more edge
cases to handle.

--
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him