Skip to content

Conversation

@kocsismate
Copy link
Member

@kocsismate kocsismate commented Jan 9, 2023

@cmb69
Copy link
Member

cmb69 commented Jan 10, 2023

Thanks for tackling this! (this PR would fix #1640 and #1784)

I'm not quite sure how to handle that method pull-up. Do we need comprehensive changelog entries? What to do with versions.xml (currently, the methods are still listed for SimpleXMLIterator there)? Should we add "alias" like pages for the old methods on SimpleXMLIterator? Or just as YOLO as in #2136?

@kocsismate
Copy link
Member Author

kocsismate commented Jan 11, 2023

I'm not quite sure how to handle that method pull-up. Do we need comprehensive changelog entries? What to do with versions.xml (currently, the methods are still listed for SimpleXMLIterator there)? Should we add "alias" like pages for the old methods on SimpleXMLIterator? Or just as YOLO as in #2136?

Yes, I had similar questions, and was hoping to get the answers from you 😅 But anyway, here are my ideas:

  • we could add a changelog entry to SimpleXMLElement that the methods in questions were moved from the iterator. IMO we don't need a changelog for SimpleXMLIterator because there is no change from its perspective
  • I'm not sure about version.xml.. I probably lean towards documenting the actual value (PHP 8). Maybe we could display a nice warning message on these pages that these methods were moved and they are available on the iterator since PHP 5.
  • My plan is to add a web-php redirection from SimpleXMLIterator to SimpleXMLElement.

Comment on lines 32 to 37
<refsect1 role="returnvalues">
&reftitle.returnvalues;
<para>
Returns the current element as a <classname>SimpleXMLElement</classname> object, or an <classname>Error</classname>
is thrown on failure.
</para>
</refsect1>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The throw an error shouldn't be mentioned in the return section but in a standard new Error section.

Comment on lines 17 to 18
Before PHP 8.0, <methodname>SimpleXMLElement::current</methodname> was declared on
<classname>SimpleXMLIterator</classname>, available as of PHP 5.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Before PHP 8.0, <methodname>SimpleXMLElement::current</methodname> was declared on
<classname>SimpleXMLIterator</classname>, available as of PHP 5.
Prior to PHP 8.0, <methodname>SimpleXMLElement::current</methodname> was only
declared on the subclass <classname>SimpleXMLIterator</classname>.

@kocsismate
Copy link
Member Author

I think I managed to address the feedback in a new commit (+ rebased to current master)

@kocsismate kocsismate merged commit dcb657b into php:master May 10, 2023
@kocsismate kocsismate deleted the stub-remove2 branch May 10, 2023 13:09
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants