Skip to content

Create exceptions ZAI seam for PHP 8 #1248

Merged
bwoebi merged 1 commit intomasterfrom
bob/zai/exceptions
Jun 2, 2021
Merged

Create exceptions ZAI seam for PHP 8 #1248
bwoebi merged 1 commit intomasterfrom
bob/zai/exceptions

Conversation

@bwoebi
Copy link
Copy Markdown
Collaborator

@bwoebi bwoebi commented May 28, 2021

Description

Using the properties seam within an exceptions seam, and use that in the php 8 source.

Readiness checklist

  • (only for Members) Changelog has been added to the release document.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

@bwoebi bwoebi added 🏆 enhancement A new feature or improvement c-extension Apply this label to issues and prs related to the C-extension labels May 28, 2021
@bwoebi bwoebi added this to the 0.61.0 milestone May 28, 2021
@bwoebi bwoebi changed the title Create exceptions zai seam for PHP 8 Create exceptions ZAI seam for PHP 8 May 28, 2021
@bwoebi bwoebi force-pushed the bob/zai/exceptions branch 2 times, most recently from 02d46df to c5767f0 Compare May 28, 2021 12:04
morrisonlevi
morrisonlevi reviewed May 28, 2021
View reviewed changes
Comment thread zend_abstract_interface/exceptions/php8/exceptions.c Outdated
@bwoebi bwoebi force-pushed the bob/zai/exceptions branch 2 times, most recently from 107104a to c88355d Compare May 28, 2021 17:12
@bwoebi bwoebi requested a review from SammyK May 28, 2021 17:13
@bwoebi bwoebi force-pushed the bob/zai/exceptions branch from c88355d to 8be5011 Compare May 28, 2021 17:19
SammyK
SammyK reviewed May 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

Once the conflicts are resolved, this LGTM. 👍 Nicely done @bwoebi.

@bwoebi bwoebi force-pushed the bob/zai/exceptions branch from 8be5011 to b6fa61a Compare May 28, 2021 21:13
@bwoebi bwoebi force-pushed the bob/zai/exceptions branch from b6fa61a to b06c660 Compare May 28, 2021 21:18
SammyK
SammyK approved these changes Jun 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

I left one question that's not a blocker for merge if you would rather keep it as-is. Thanks @bwoebi! 👍

Comment thread zend_abstract_interface/exceptions/exceptions_common.h

#if PHP_VERSION_ID < 70100

#define ZEND_STR_MESSAGE "message"
Copy link
Copy Markdown
Contributor

@SammyK SammyK Jun 1, 2021

Choose a reason for hiding this comment

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

Now that I think about this a bit more, do you think it would make sense to remove exceptions_common.h since each major PHP version will have its own php<n>/exceptions.h header?

Copy link
Copy Markdown
Collaborator Author

@bwoebi bwoebi Jun 2, 2021

Choose a reason for hiding this comment

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

I'm not sure yet. I think we'll see what makes most sense once we actually implement the php 5 and 7 seams. I think it should be changed (or not) at the point of implementing these.

@bwoebi bwoebi merged commit 8d28b03 into master Jun 2, 2021
@bwoebi bwoebi deleted the bob/zai/exceptions branch June 2, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@morrisonlevi morrisonlevi morrisonlevi left review comments

+1 more reviewer

@SammyK SammyK SammyK approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c-extension Apply this label to issues and prs related to the C-extension 🏆 enhancement A new feature or improvement

Projects

None yet

Milestone

0.61.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bwoebi @morrisonlevi @SammyK