Minds / engine

mirror of https://gitlab.com/minds/engine
https://minds.com
GNU Affero General Public License v3.0
200 stars 94 forks source link

Push msgpack implementation out of PHP userland #8

Closed DaSourcerer closed 7 years ago

DaSourcerer commented 7 years ago

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 commented 7 years ago

keep it simple stupid, nais!

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

markharding commented 7 years ago

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

DaSourcerer commented 7 years ago

@markharding Could that be related to socketio/socket.io-redis#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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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.