search

auth0 / wordpress

WordPress Plugin for Auth0 Authentication
https://auth0.com/docs/cms/wordpress
MIT License
154 stars 96 forks source link

Basic trio of tests #786

Closed szepeviktor closed 4 years ago

szepeviktor commented 4 years ago

Please introduce my basic trio as authentication is a delicate matter.

  1. syntax check vendor/bin/parallel-lint --exclude vendor/ .
  2. coding standard ✔️
  3. static analysis, see #785

These should be part of every CI.

joshcanhelp commented 4 years ago

@szepeviktor - Are we OK to close this now?

szepeviktor commented 4 years ago

If you are not wiling to start using PHPStan, add it to CI, and fix some more problems.

szepeviktor commented 4 years ago 
viktor@mail:~/src/wp-auth0$ vendor/bin/phpstan analyze -l 2
Note: Using configuration file /home/viktor/src/wp-auth0/phpstan.neon.dist.
 55/55 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------
  Line   lib/WP_Auth0_LoginManager.php
 ------ --------------------------------------
  274    Variable $user might not be defined.
 ------ --------------------------------------

 ------ -------------------------------------------------------
  Line   lib/WP_Auth0_Nonce_Handler.php
 ------ -------------------------------------------------------
  83     Unsafe usage of new static().
         💡 Consider making the class or the constructor final.
 ------ -------------------------------------------------------

 ------ ---------------------------------------
  Line   lib/WP_Auth0_WooCommerceOverrides.php
 ------ ---------------------------------------
  40     Function wc_get_page_id not found.
 ------ ---------------------------------------

 ------ -------------------------------------------------
  Line   lib/api/WP_Auth0_Api_Client_Credentials.php
 ------ -------------------------------------------------
  84     Access to an undefined property object::$scope.
 ------ -------------------------------------------------

 ------ -------------------------------------------------------------
  Line   lib/initial-setup/WP_Auth0_InitialSetup_Consent.php
 ------ -------------------------------------------------------------
  112    Cannot access property $client_id on array|object|true.
  113    Cannot access property $client_secret on array|object|true.
  115    Cannot access property $client_id on array|object|true.
 ------ -------------------------------------------------------------
joshcanhelp commented 4 years ago

I am willing to accept more PRs 😄 I wasn't sure if #785 was the extent of what you wanted to do or if there was more.

szepeviktor commented 4 years ago

That PR was a bunch of fixes on Level 2, no integration.

szepeviktor commented 4 years ago
szepeviktor commented 4 years ago

e.g. in WP_Auth0_Api_Client::create_client we may go two ways with @return bool|object|array

joshcanhelp commented 4 years ago

Variable $user need cleaner, testable logic

I'm open to suggestions. That entire method could use a better approach and I'm wary about changing logic just to pass a code style test. Feels like there are better places to focus effort.

WP_Auth0_Nonce_Handler needs to be final to be safe

That's extended by WP_Auth0_State_Handler (which is final)

wc_get_page_id could be added as a stub

Or a guard on that method like !function_exists('wc_get_page_id')

making an object from JSON data could be replaced with array output, stray objects are dangerous and not testable

All of the stuff in WP_Auth0_Api_Client_Credentials->handle_response() is internal so switching to an array is fine. It should return string or null so that wouldn't be breaking.

WP_Auth0_Api_Client::create_client we may go two ways

Everything in that class should eventually move to how it's being done in, say, WP_Auth0_Api_Change_Email. I would not spend any time refactoring methods in that class.

A lot of our error handling is legacy/inconsistent, no arguments there! Keep in mind, though, that the developer interaction with this plugin is far less often calling these class methods directly. Also, WordPress is far from modern OOP or modern PHP at all so there is some leeway there.

szepeviktor commented 4 years ago

I'm wary about changing logic just to pass a code style test.

Excuse me!! PHPStan is a static analysis tool that has way more eyes than we humans do, or PHPCS does.

Wait for Level MAX.

szepeviktor commented 4 years ago

Keep in mind, though, that the developer interaction with this plugin is far less often calling these class methods

I tell you my secret. Everything I do to this plugin is to make it testable with PHPStan, so my inner QA engineer can install it at my clients.

szepeviktor commented 4 years ago

Conclusion

Many of the WordPress-related abnormalities could be solved by installing 840 lines of code: https://github.com/thephpleague/container

(or another container)

joshcanhelp commented 4 years ago

I'm open to seeing PRs that address this but refactoring code, particularly code that is not tested and has been around for a while, is not without risk. I understand that you want to improve the code base and I really appreciate that. We have to weigh the benefit of that improvement with the risk of making changes.

szepeviktor commented 4 years ago

Introducing a DI container could reduce our code as all lines dealing with instantiation could be simply removed.

joshcanhelp commented 4 years ago

Could you help me understand exactly what that means, in this context? Would we need breaking changes? Would the DX change? What are the main benefits we would get out of introducing that?

szepeviktor commented 4 years ago

Would we need breaking changes?

No, classes would instantiate automatically behind the scenes.

Would the DX change?

No, class instantiation is not the job of a developer.

What are the main benefits we would get out of introducing that?

szepeviktor commented 4 years ago

things like

https://github.com/auth0/wp-auth0/blob/709ec491069d307b6f80c8268aacd7c3115c9dbf/lib/WP_Auth0_Options.php#L45-L53

could vanish right away

joshcanhelp commented 4 years ago

Sounds like a solid improvement, happy to take a look at a PR. I would start small, though, so the review burden isn't too high.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️