Skip to content

Change DOMCharacterData::appendData return type to true #10690

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

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

othercorey
Copy link
Contributor

@Girgias

This function always returns true. Updating stub as suggested in doc-en review php/doc-en#2187.

@@ -485,7 +485,7 @@ class DOMCharacterData extends DOMNode implements DOMChildNode
public ?DOMElement $nextElementSibling;

/** @tentative-return-type */
public function appendData(string $data): bool {}
public function appendData(string $data): true {}
Copy link
Member

Choose a reason for hiding this comment

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

@Girgias Do you have an idea when and how we could migrate these return types to void? Does it make sense to first migrate them to tentative true?

Copy link
Member

Choose a reason for hiding this comment

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

Probably only PHP 9.0. But marking them as true indicates that the return value is pointless.

The issue with going from true to void is that if the return value is used it goes from true to null which if checked in a condition will now hit the other branch :/

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see the problem :) I was mainly interested in whether you see the benefit of the slight BC break of changing non-final method return types to true before finally marking them as void.

Once, I came up with an idea to check whether the return type is used (RETURN_VALUE_USED): if so, then we could add a deprecation notice. I think this is the best possibility we have, but Nikita wasn't exactly happy with it back then because it doesn't work in all cases.

@Girgias
Copy link
Member

Girgias commented Feb 25, 2023

Can you add an entry to UPGRADING to signal the change of the tentative return type?

@Girgias Girgias self-assigned this Feb 25, 2023
@othercorey
Copy link
Contributor Author

Can you add an entry to UPGRADING to signal the change of the tentative return type?

Added.

@Girgias Girgias merged commit e1967ca into php:master Feb 25, 2023
@othercorey othercorey deleted the appenddata-true branch February 25, 2023 23:28
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