littlebizzy / slickstack

Lightning-fast WordPress on Nginx
https://slickstack.io
GNU General Public License v3.0
635 stars 111 forks source link

Blacklisted plugins philosophy and why flexible-shipping-ups is listed #9

Closed dyszczo closed 2 years ago

dyszczo commented 5 years ago

Hi,

As I was playing with SlickStack, I found that WP Desk company along with its plugin flexible-shipping-ups was marked as blacklisted because of "poor coding, serious errors, better options exist". It so happens that I can fix this plugin. Could you please provide me with more information on what exactly is this plugin doing wrong and what can be done to avoid being blacklisted? Also, as far as I know, there is no other free option for UPS live rate calculation other than this plugin.

jessuppi commented 5 years ago

Hmm good question. I'm not sure I have documented the original reason on this plugin, I think perhaps we got it confused with another "UPS WooCommerce Shipping" errors e.g.

PHP Fatal error: Uncaught Error: Call to undefined function WC() in /var/www/html/wp-content/plugins/ups-woocommerce-shipping/includes/class-wf-shipping-ups-admin.php:205

...and after looking at recent reviews, we believed there were other issues as well:

https://wordpress.org/plugins/flexible-shipping-ups/#reviews

For now I will remove this plugin from our blacklist. Thanks for commenting.

jessuppi commented 5 years ago

Actually it appears you have a non-dismissable nag notice asking users to enable tracking, which is probably against WordPress.org guidelines (besides the general annoyance).

I recommend and request not hijacking the WP Admin in any way... and 99% of plugins don't need to be tracking their users for general usage statistics.

Will keep it whitelisted for now, but seems multiple of your plugins are doing hijacky-ness...

dyszczo commented 5 years ago

Thanks very much for adding the plugin to the whitelist.

As a backend dev I understand perfectly why this notice is a complete abomination, but as far is I know you can dismiss it quite simply by closing it so it should be a one-time dirturbance. The data that are tracked are used to grasp what functionalities are really used by users (mostly to prioritize our backlog), and what really works for users when we use A/B testing in a plugin. Surprisingly for me that is not the info you can easly get from users as they usually don't know or don't really care till something is broken :)

As for reviews, well... we have dropped the ball and passed a really nasty bug that appears only when certain other plugins are active :/ The lesson has been learned and we've added more integration/functional tests to deal with it but the matter is not that easy to solve. Mostly because WordPress made using composer a real pain in the ass.

I've seen that dashboard-cleanup define some const like DISABLE_NAG_NOTICES. I'm not sure if that is a good idea but we can add a condition to not nag more demanding users who have declared something like that const.

jessuppi commented 5 years ago

As a backend dev I understand perfectly why this notice is a complete abomination, but as far is I know you can dismiss it quite simply by closing it so it should be a one-time dirturbance.

When we tested yesterday, we weren't able to dismiss the nag notice.

The data that are tracked are used to grasp what functionalities are really used by users (mostly to prioritize our backlog), and what really works for users when we use A/B testing in a plugin.

But any plugin author could say that, and most sites have 25-50 plugins or more... surely tracking general usage is not something WordPress as a community wants to standardize. Besides the privacy and bloat issues, its adding more security/performance concerns.

Implementing DISABLE_NAG_NOTICES would definitely be appreciated by advanced users, and show the community that you're making good faith efforts.

Eventually we may even feature plugin authors who make these sorts of ethical efforts:

szepeviktor commented 5 years ago

@dyszczo It takes couple of minutes to take a look at a plugin with szepeviktor/phpstan-wordpress

here is the first try on the config for flexible-shipping-ups plugin

phpstan.neon

# Install phpstan/phpstan-shim and szepeviktor/phpstan-wordpress into a renamed vendor dir
# "config": { "vendor-dir": "analyze" }
# analyze/bin/phpstan analyze

includes:
    - phar://phpstan.phar/conf/bleedingEdge.neon
    - analyze/szepeviktor/phpstan-wordpress/extension.neon
parameters:
    level: 4
    paths:
        - %currentWorkingDirectory%/inc/
        - %currentWorkingDirectory%/classes/
    excludes_analyse:
        - %currentWorkingDirectory%/inc/wpdesk-tracker/views/
        - %currentWorkingDirectory%/classes/views/
    autoload_files:
        - %currentWorkingDirectory%/vendor/autoload.php
        - %currentWorkingDirectory%/wc-stubs.php
    autoload_directories:
        - %currentWorkingDirectory%/inc/
        - %currentWorkingDirectory%/classes/
        - %currentWorkingDirectory%/vendor/wpdesk/wp-basic-requirements/src/
    ignoreErrors:
        # Uses func_get_args()
        - '#^Function add_query_arg invoked with [123] parameters?, 0 required\.$#'

wc-stubs.php is generated by https://github.com/szepeviktor/phpstan-wordpress/issues/4

jessuppi commented 5 years ago

@dyszczo It takes couple of minutes to take a look at a plugin with szepeviktor/phpstan-wordpress ... here is the first try on the config for flexible-shipping-ups plugin

Wow, that's some nice looking sorcery. I'm not a PHP wizard myself, to be honest...

If there were easy to understand "red flags" or a grading system, that would be kinda interesting to refer plugin authors to as some general feedback on their code. Having WordPress-specific warnings would probably be too difficult to maintain I guess, but something that can at least generate some type of grading system would be interesting, like A, B, C, etc.

I would guess the majority of WP developers don't use Composer and probably are hacking together all kinds of PHP snippets, so something easy to generate (online tool?) would be amazing.

Anyway cool stuff :)

szepeviktor commented 5 years ago

@jessuppi see

  1. https://github.com/nunomaduro/phpinsights
  2. https://themecheck.info/ - https://wordpress.org/plugins/theme-check/
  3. https://coderisk.com/
  4. https://wpvulndb.com/
  5. https://github.com/WordPress/WordPress-Coding-Standards/tree/develop/WordPress/Sniffs/Security
  6. and viktor@szepe.net to run various quality code bases
dyszczo commented 5 years ago

Wow it looks like PHPStan is so much more than standard PHPStorm inspections even with code sniffer integration. I will allow myself to grab the @szepeviktor phpstan config and add it to our CI - we are using gitlab CI :) Thank you @szepeviktor for the extension :+1:

szepeviktor commented 5 years ago

You're welcome.