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.

Sign up for GitHub

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

Jump to bottom

Convert resource to object in Sysvshm extension #5499

Closed
wants to merge 10 commits into from
Closed

Convert resource to object in Sysvshm extension #5499

wants to merge 10 commits into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Apr 30, 2020 •
edited
Loading

No description provided.

@kocsismate kocsismate marked this pull request as draft April 30, 2020 09:56
@kocsismate kocsismate force-pushed the sysvshm-resource branch 3 times, most recently from a4bd83b to 14ed430 Compare April 30, 2020 11:19
@kocsismate kocsismate marked this pull request as ready for review April 30, 2020 11:20
@kocsismate kocsismate force-pushed the sysvshm-resource branch 2 times, most recently from 72fe90f to 68d63d3 Compare May 1, 2020 17:22
nikic
nikic reviewed May 6, 2020
View reviewed changes
ext/sysvshm/sysvshm.c Outdated Show resolved Hide resolved
ext/sysvshm/sysvshm.c Show resolved Hide resolved
ext/sysvshm/sysvshm.stub.php Outdated Show resolved Hide resolved
@kocsismate kocsismate force-pushed the sysvshm-resource branch 3 times, most recently from f2b4b70 to 3eac56a Compare May 7, 2020 07:31
@kocsismate kocsismate force-pushed the sysvshm-resource branch 4 times, most recently from f0c4085 to 143c535 Compare May 17, 2020 21:17
@kocsismate kocsismate force-pushed the sysvshm-resource branch 4 times, most recently from ab91f89 to c33db95 Compare June 11, 2020 07:13
@cmb69
Copy link
Member

cmb69 commented Jun 16, 2020

I just noticed that this overlaps with PR #3235.

nikic
nikic reviewed Jun 16, 2020
View reviewed changes
UPGRADING Outdated Show resolved Hide resolved
ext/sysvshm/sysvshm.c Show resolved Hide resolved
ext/sysvshm/tests/003.phpt Outdated Show resolved Hide resolved
@kocsismate
Copy link
Member Author

kocsismate commented Jun 16, 2020 •
edited
Loading

@cmb69 Thanks for the heads up! Unfortunately, I didn't notice this before. However, I think Michał's PR is quite much outdated.

Though, should we get inspiration from his work, and use SysvSharedMemoryBlock (or SysvSharedMemorySegment) instead of SysvSharedMemory?

@cmb69 cmb69 mentioned this pull request Jun 16, 2020
Closed
@brzuchal
Copy link
Contributor

@cmb69 my PR is way outdated. AFAIR it took me a while to figure out proper naming of classes and functions so feel free to reuse them.

nikic
nikic reviewed Jun 16, 2020
View reviewed changes
ext/sysvshm/sysvshm.c Outdated Show resolved Hide resolved
ext/sysvshm/sysvshm.c Outdated Show resolved Hide resolved
nikic
nikic approved these changes Jun 16, 2020
View reviewed changes
ext/sysvshm/sysvshm.c Outdated Show resolved Hide resolved
ext/sysvshm/sysvshm.c Outdated Show resolved Hide resolved
@php-pulls php-pulls closed this in 2e18b30 Jun 16, 2020
@kocsismate kocsismate deleted the sysvshm-resource branch June 16, 2020 14:39
@brzuchal
Copy link
Contributor

I didn't take a deserved look to the patch to agree with closing my outdated PR enough. What you propose here is just a replacement for a resource and delivers an object with no behaviour - IMO that doesn't make sense to bring a class with no behaviour if we can! most of read-write functions could be added to the OOP API on introduced class.

@kocsismate
Copy link
Member Author

kocsismate commented Jun 16, 2020 •
edited
Loading

didn't take a deserved look to the patch to agree with closing my outdated PR enough.

Then sorry for the premature close, and feel free to reopen it if you desire to.

I hope my previous reply (#3235 (comment)) clears your doubts. I think it makes sense to separate these changes. Especially, because the migration by using an "opaque" object didn't need any RFC (or at least comments of a wider audience), while a new API does.

@brzuchal
Copy link
Contributor

@kocsismate no worries. I'll take advantage of proposing similar API on a fresh PR soon.

@carusogabriel carusogabriel added this to the PHP 8.0 milestone Jun 16, 2020
@Girgias Girgias mentioned this pull request Jul 27, 2020
Open
41 tasks
@jrfnl jrfnl mentioned this pull request Aug 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 left review comments

@Girgias Girgias Girgias left review comments

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
PHP 8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@kocsismate @cmb69 @brzuchal @nikic @Girgias @carusogabriel