Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 3, 2025

This simplifies some of the logic and makes the assumptions clear

@Girgias Girgias force-pushed the phar-spprintf-errors-refacto branch 2 times, most recently from dabaffb to c618444 Compare August 3, 2025 22:25
@Girgias Girgias marked this pull request as ready for review August 3, 2025 22:26
@Girgias Girgias requested a review from nielsdos August 3, 2025 22:26
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Looks mainly good. For the future it would be good to defer zend_string refactors to another PR because it's just another thing to check, and I already had to reason "recursively" to check the validity for these changes, so less context switching is better

@nielsdos
Copy link
Member

CI failure is legit. It's probably because of code that was added/changed in the meantime.

@Girgias
Copy link
Member Author

Girgias commented Oct 13, 2025

CI failure is legit. It's probably because of code that was added/changed in the meantime.

Yeah, but I wanted to extract some of the changes into a different PR which is now done in #20162

I'll rebase after that is merged and fix the NULL pointer attributes :)

@Girgias Girgias force-pushed the phar-spprintf-errors-refacto branch from 6b0acde to 4e923ee Compare October 15, 2025 21:57
@Girgias Girgias requested a review from nielsdos October 15, 2025 21:58
@Girgias Girgias force-pushed the phar-spprintf-errors-refacto branch from 4e923ee to 4892ece Compare October 16, 2025 10:28
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

The test changes and C changes should likely be committed separately

@Girgias
Copy link
Member Author

Girgias commented Oct 16, 2025

Right, I forgot about them as I used them to debug my test failures. But those were just caused due to my OpenSSL not being able to verify the signature

This simplifies some of the logic and makes the assumptions clear
@Girgias Girgias force-pushed the phar-spprintf-errors-refacto branch from 4892ece to aa3d4d5 Compare October 16, 2025 20:02
@Girgias Girgias merged commit 9a24c6a into php:master Oct 16, 2025
9 of 10 checks passed
@Girgias Girgias deleted the phar-spprintf-errors-refacto branch October 16, 2025 20:02
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.

2 participants