Skip to content
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

ext/zend_test: Move object handler test objects to their own file #11852

Merged
merged 1 commit into from Aug 2, 2023

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 1, 2023

I think this makes sense to split up as setting up an object to test one of the handler requires quite a bit of boilerplate that pollutes the base test file.

This would also reduce the amount of conflicts, as every time I rebased PRs introducing such objects I had permanent conflicts :/

Kinda related to #9218 as I want to work on this again.

@iluuu1994
Copy link
Member

iluuu1994 commented Aug 2, 2023

Moving things can also greatly increase conflicts. 🙂 Adding things to lower branches is a guaranteed conflict. I'm generally not fond of moving things.

@Girgias
Copy link
Member Author

Girgias commented Aug 2, 2023

Moving things can also greatly increase conflicts. slightly_smiling_face Adding things to lower branches is a guaranteed conflict. I'm generally not fond of moving things.

I can backport this to 8.1, I should have create this file the first time I added an object handler test to be honest.

@Girgias
Copy link
Member Author

Girgias commented Aug 2, 2023

Moving things can also greatly increase conflicts. slightly_smiling_face Adding things to lower branches is a guaranteed conflict. I'm generally not fond of moving things.

I can backport this to 8.1, I should have create this file the first time I added an object handler test to be honest.

Actually, those objects only exist in php-src rn.

@iluuu1994
Copy link
Member

iluuu1994 commented Aug 2, 2023

I guess that's fine. Adding a new file can also be somewhat annoying because it requires re-running autoconf, especially when bisecting. None of these are particularly problematic for this PR though due to being restricted to zend_test. Anyway, I'm not blocking this, just stating my opinion.

Actually, those objects only exist in php-src rn.

I assume you mean master? In that case it's less problematic I guess.

@Girgias
Copy link
Member Author

Girgias commented Aug 2, 2023

I guess that's fine. Adding a new file can also be somewhat annoying because it requires re-running autoconf, especially when bisecting. None of these are particularly problematic for this PR though due to being restricted to zend_test. Anyway, I'm not blocking this, just stating my opinion.

Actually, those objects only exist in php-src rn.

I assume you mean master? In that case it's less problematic I guess.

Yes I did mean master 😅

@Girgias Girgias merged commit 65a02f4 into php:master Aug 2, 2023
12 checks passed
@Girgias Girgias deleted the zend_test_object_handlers branch August 2, 2023 17:52
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 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.

None yet

2 participants