Skip to content

Conversation

@DaSourcerer
Copy link

Instead of using a userland implementation of msgpack, taking advatage of the PHP pecl module may prove to be faster and comes with a real chance of decreasing Minds' operational costs. For this to work, the php-msgpack package has to be installed.

Further reading: pecl::msgpack module info

@001101
Copy link

001101 commented Feb 14, 2017

keep it simple stupid, nais!

btw. as i seen you like yii2, do you know humhub? https://github.com/humhub/humhub

@markharding
Copy link
Member

@edgebal I recall we had problems with php's msgpack and socket.io conflict?

@DaSourcerer
Copy link
Author

@markharding Could that be related to socketio/socket.io-redis-adapter#17?

@001101 I somewhat lost track of Yii2 after PHP stopped being my bread and butter. But I still remember some of the contributors of that project.

@001101
Copy link

001101 commented Feb 14, 2017

@DaSourcerer It is very nais and evolving very much, big potential, you should give it a try. What is now your bread and butter?

@edgebal
Copy link
Contributor

edgebal commented Feb 14, 2017

@markharding We had a versioning issue. There was some kind of incompatibility with the MsgPack, Socket.IO and Redis versions Minds was using. Anyhow, I think it's worth checking it out.

cc/ @DaSourcerer

@DaSourcerer
Copy link
Author

@edgebal IIRC the PECL extension is following a newer spec for MsgPack than socket.io. Per the issue I linked, I think this can be addressed by upgrading the affected lib.

Also: No need to cc me when I opened the PR ;)

@DaSourcerer
Copy link
Author

Ah, there's another caveat: msgpack.php_only is set to On by default. If I see correctly, this controls how PHP objects are serialized. Switching it to Off should prevent interoperability issues.

@markharding
Copy link
Member

We had to do this polyfill due to issues with the libraries, which I dont think are fixed yet. I'm tempted to suggest that if this isn't broken, then lets not fix it.

michealmueller added a commit to michealmueller/engine that referenced this pull request Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants