joedolson / wp-to-twitter

XPoster - Post to X.com and Mastodon
https://xposterpro.com
GNU General Public License v3.0
8 stars 3 forks source link

Prefix `vendor` directory using php-scoper #6

Closed rob006 closed 1 year ago

rob006 commented 1 year ago

Since 4.0.0 this extension bundles some popular libraries. In PHP each class and function can be defined only once, so this will create errors if there is other plugin doing the same, or someone is already using composer to manage his WP installation.

Common solution to solve this is to prefix all vendor libraries using something like https://github.com/humbug/php-scoper - this changes names of symbols in vendor directory ensuring they're unique. For example https://github.com/Yoast/wordpress-seo is doing this.

https://github.com/Yoast/wordpress-seo/blob/07be295d7c53635dc0afb06782f47c33033ed739/composer.json#L109-L124

joedolson commented 1 year ago

OK, I thought maybe 4.0.1 milestone, but on reading the scope of work required to implement php-scoper in a WordPress plugin, I'm moving this to a later milestone. This looks like a pretty major project to implement, potentially requiring me to refactor the entire codebase.

I'm basing this on the info here: https://graphql-api.com/blog/graphql-api-for-wp-is-now-scoped-thanks-to-php-scoper/#heading-show-me-the-real-stuff, if you know an easier path, don't hesitate to suggest it.

rob006 commented 1 year ago

It sounds quite urgent to me. It definitely blocks update in my case, and I'm pretty sure that problems like https://wordpress.org/support/topic/critical-error-on-test-xposter/ are results of unscoped dependencies.

Also, it does not look that complicated. In https://graphql-api.com/blog/graphql-api-for-wp-is-now-scoped-thanks-to-php-scoper/ the main source of problems were dependencies that references WP functions and classes - this should not be a problem here. And vendor dependencies are referenced only in two places in plugin itself.

rob006 commented 1 year ago

@joedolson I created a PR with scoper support: https://github.com/joedolson/wp-to-twitter/pull/7 - it required only two changes in plugin classes.

joedolson commented 1 year ago

I'm pretty sure the error on https://wordpress.org/support/topic/critical-error-on-test-xposter/ is caused by incomplete handling of Exceptions, fixed in https://github.com/joedolson/wp-to-twitter/commit/0b14ef74f41c21c82ada0881b213947840964bbf.

Thanks for the PR; I'll take a look at it. Can you tell me why you removed the line setting symfony/deprecation-contracts to 2.5.2? I'd added that because versions later than 2.5.2 require PHP 8.0+, and I really need to support PHP 7.4.

rob006 commented 1 year ago

I replaced this with overwriting PHP version in platform settings - it should be more readable and reliable than using constraint on single package:

        "platform": {
            "php": "7.4.30"
        }
joedolson commented 1 year ago

I've done a bit of testing of this, and it seems to work fine. I'm going to go ahead and merge it for easier testing, see if anything crops up. But I'll probably ship it tomorrow. Given that I'm moving this from "doesn't work at all" to "works in most* scenarios", I'm not going to worry too much about it, as long as I'm not seeing fatals.