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

Prepare for working with newer versions of OpenSwoole. #128

Open
wants to merge 3 commits into
base: 4.10.x
Choose a base branch
from

Conversation

slaff
Copy link

@slaff slaff commented Feb 5, 2024

Q A
Documentation no
Bugfix yes
BC Break no
New Feature yes
RFC no
QA no

Make Mezzio Compatible with OpenSwoole v 22.0 and newer

This PR adds initial compatibility with OpenSwoole v22.0 and newer.
The changes in the PR are inspired by @Xerkus work and all credit goes to him.
This PR is related to #110.

TODO

  • Update the migration document and mention that custom fixes are no longer needed to support openswoole >= v22.0

@slaff slaff force-pushed the feat/openswoole-v22-compat branch from 16f59a1 to c5ec024 Compare February 5, 2024 13:55
composer.json Outdated Show resolved Hide resolved
@Xerkus
Copy link
Member

Xerkus commented Feb 5, 2024

I don't think changing autoloading rules affects lock file. Those fields are not used for hash afaik

Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>

// @see https://github.com/mezzio/mezzio-swoole/issues/110#issuecomment-1500174967
// Override the swoole_set_process_name function
if (version_compare(phpversion('openswoole'), '22.0.0', '>=')) {
Copy link
Member

Choose a reason for hiding this comment

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

This should check if openswoole extension is installed first

Copy link
Member

Choose a reason for hiding this comment

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

Actually, if openswoole users added this workaround to their code this will cause errors. So this should also be wrapped in function_exists.

Verification is needed that vendor autoload files are included after root composer.json autoloading rules

Copy link
Author

Choose a reason for hiding this comment

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

Actually, if openswoole users added this workaround to their code this will cause errors.

If there is a migration document I can document this. Ideally this package should take care of such differences and not the end user. I guess end users will be more than happy to have mezzio-swoole working out of the box without the need for them to provide custom code.

@Xerkus
Copy link
Member

Xerkus commented Feb 5, 2024

@slaff I am not an author here, btw.

@Xerkus Xerkus linked an issue Feb 5, 2024 that may be closed by this pull request
@slaff
Copy link
Author

slaff commented Feb 5, 2024

I am not an author here, btw.

@Xerkus thank you anyway. If you can, please, tell me whom shall I credit and I will put their names.

Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>
@slaff slaff force-pushed the feat/openswoole-v22-compat branch from f2b2617 to 01a247d Compare February 5, 2024 15:35
Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>
@madalinignisca
Copy link

I am confused as to why the project is introducing a compatibility layer for the current version of OpenSwoole, and it is not refactoring to continue 1st party with OpenSwoole, and add a compatibility layer in case Swoole is used, if the effort is worth it.

The Mezzio Swoole documentation instructs users to use OpenSwoole, not Swoole.

If OpenSwoole 24.x, 26.x will diverge more from Swoole (which will happen), wouldn't users be aware that the package is focused on Swoole, opposite to what they are instructed by docs for OpenSwoole?

I'd be happy to see the package in newer versions requiring openswoole > 22.

@slaff
Copy link
Author

slaff commented Feb 8, 2024

I'd be happy to see the package in newer versions requiring openswoole > 22.

Me too. But it is up to the project leaders and contributors to decide which direction to go.

@alexmerlin
Copy link
Member

@slaff Anything else to add to this PR?

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.

Compatibility with openswoole 22
7 participants