Feature Request: Please address this plugin's conflicts with `php-stubs/wordpress-stubs`

montchr commented 1 year ago

Terms

[X] I have read the guidelines for Contributing to Roots Projects
[X] This request is not a duplicate of an existing issue
[X] This is not a personal support request that should be posted on the Roots Discourse community

Summary

It is currently not possible to use phpstan with phpstan-wordpress and the wordpress-stubs packages when installing roots/wp-password-bcrypt via composer.json without resorting to some very hacky (and unreliable/repetitive/frustrating) workarounds.

The full issue is described in https://github.com/szepeviktor/phpstan-wordpress/issues/41.

Motivation

Why are we doing this?

To reduce the frustration of working with WordPress, to improve compatibility, and to facilitate reproducible development tasks in local and CI workflows.

Please read:

I am aware this has been mentioned indirectly in the past, but it appears that there has been little consideration given to making a change in this plugin despite the numerous accounts over the years. I am referring specifically to https://github.com/roots/wp-password-bcrypt/issues/11, but there are other examples.

It would be great if roots/wp-password-bcrypt could find a way to improve compatibility with these other libraries, because both this plugin and those libraries have been positive improvements to the WordPress ecosystem that unfortunately do not mix well.

What use cases does it support?

Using the widely-used phpstan static analysis tooling with WordPress integration via https://github.com/szepeviktor/phpstan-wordpress
Using this plugin via Composer installation

What is the expected outcome?

Developers and CI pipelines should be able to run phpstan analyse when this plugin is installed via composer.json and the WordPress stubs are loaded without any conflict.

I encourage the authors of this plugin to listen to the feedback from other developers who have encountered such conflicts and have given specific suggestions as to a fix.

For example:

Check if function exists in wordpress-stubs.php · Issue #70 · php-stubs/wordpress-stubs

I've given a couple of workshops on WordPress+PHPStan and both times this exact problem was the first to arise. We determined that the most correct solution is for the Bedrock bcrypt library to wrap its functions in function_exists() checks because it has no way to ensure that another definition of one of those functions isn't loaded by another plugin. I'm not sure why that is not the case.

Potential conflicts / foreseeable issues

Unknown. I am only a user of both tools reporting my experience and, more importantly, linking the two GitHub issues together because it looks like there's only been isolated conversations in each repo about this issue.

Additional Context

No response

timnolte commented 3 weeks ago

This issue is still a problem. How are Roots sites even being tested for code quality when this issue exists. Looking at the history of this package and it seems like it is really not even properly maintained anymore.

swalkinshaw commented 3 weeks ago

What a shitty comment 👍

None of us are familiar with php-stubs/wordpress-stubs and from reading https://github.com/szepeviktor/phpstan-wordpress/issues/41 it doesn't seem there's an obvious solution? This is an open source repo so it would be great if someone who is more familiar with the wordpress-stubs and the issue could contribute a solution.

timnolte commented 3 weeks ago

What a shitty comment 👍

None of us are familiar with php-stubs/wordpress-stubs and from reading https://github.com/szepeviktor/phpstan-wordpress/issues/41 it doesn't seem there's an obvious solution? This is an open source repo so it would be great if someone who is more familiar with the wordpress-stubs and the issue could contribute a solution.

It's a legitimate question. If PHPStan isn't being used then I have to assume there are other code quality tools being used that are apparently better than PHPStan.

It's also, however, a fact that there don't appear to be any commits to the repository for the last 7 months. So I'm assuming then it just doesn't need any updates as it works fine with whatever code quality tools that Roots is using and is still fully compatible with the latest WordPress Core. If the answer is just that PHPStan is not supported and there is no plan to support it then so be it, and we can close this issue as won't fix.

oxyc commented 3 weeks ago

One solution is to package this as a mu-plugin instead of a library https://github.com/generoi/wp-password-bcrypt/commit/0903a48363a1a6fce96b9815175ec1f1fee41ffc.

I felt this was a controversial change so haven't opened up a PR but happy to do so

Reading the filedoc of pluggable.php they specifically say plugins so it would align better with WordPress expectations.

swalkinshaw commented 3 weeks ago

😅 this package started out as an mu-plugin: https://github.com/roots/wp-password-bcrypt/pull/4

I have zero recollection why it was changed

retlehs commented 3 weeks ago

The only reason I can think of is that we figured if folks are already using Composer, we could just keep everything in the vendor directory instead of the plugin having to exist in the mu-plugins directory and showing up in the must-use plugins list 🤔

johnbillion commented 4 days ago

Previously: #33

roots / wp-password-bcrypt