Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix get_object_vars() for non-hooked props in hooked prop iter #16805

Closed
wants to merge 1 commit into from

Conversation

iluuu1994
Copy link
Member

Very confusingly, the zend_hash_update_ind() variant unwraps indirects, rather than creating them. Don't use _zend_hash_append_ind() because the property might already exist.

Fixes GH-16725

Very confusingly, the zend_hash_update_ind() variant unwraps indirects, rather
than creating them. Don't use _zend_hash_append_ind() because the property might
already exist.

Fixes phpGH-16725
@iluuu1994
Copy link
Member Author

@php/release-managers-84 Since we're past the last RC, are you ok merging this for GA? The release process says:

https://github.com/php/policies/blob/main/release-process.rst

The GA release MUST be released from the last Release Candidate tag (RC4 or later). There MUST NOT be any changes between the last Release Candidate tag and the GA tag (with exception to files such as NEWS and other files where the PHP version number must change for the GA release).

I don't think this change should require another RC, but delaying it until 8.4.1 also seems unnecessary. WDYT?

@Girgias
Copy link
Member

Girgias commented Nov 15, 2024

We could always created a new RC, no?

@iluuu1994
Copy link
Member Author

That would delay the release by two weeks. I don't think that's necessary here.

@bwoebi
Copy link
Member

bwoebi commented Nov 16, 2024

I think we should adapt the policy in general that trivial&well-understood bugfixes to new features introduced in the final RC as well as fixing regressions introduced shall always be permissible. Non-trivial bugfixes shall be cause for a new RC (or be delayed to the next patch release, depending on severity/impact).

@bukka
Copy link
Member

bukka commented Nov 17, 2024

As mentioned privately I fully agree with what Bob said. There should be just an addition that it should be in discretion of RM like all other case as someone will have to decide what is "trivial & well-understood". This rule can be applied immediately IMO as the current rules are not completely approved.

@iluuu1994
Copy link
Member Author

Great, thanks @bukka! @php/release-managers-84 Please consider this for 8.4 GA then.

@cmb69
Copy link
Member

cmb69 commented Nov 18, 2024

I'm counting 13 bugfixes for PHP 8.4.0 in https://github.com/php/php-src/blob/PHP-8.4/NEWS. Have these all been approved by RMs? Or will PHP 8.4.0 be cut from another branch as PHP-8.4 (in which case NEWS should have been updated to say PHP 8.4.1).

@iluuu1994
Copy link
Member Author

@cmb69 According to our discussion, it would be branched from the last RC tag, and then the relevant changes would be cherry-picked.

@cmb69
Copy link
Member

cmb69 commented Nov 18, 2024

Thank you! Then I can apply to PHP-8.4. :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants