Trigger deprecation and fallback to `none` strategy on configuration

rvanlaak commented 1 year ago

Checklist

[X] This can be reproduced in the quickstart sample application.
[X] I have reviewed the README.md and have not found a solution.
[X] I have reviewed the EXAMPLES.md and have not found a solution.
[X] I have searched previous issues and have not found a solution.
[X] I have searched the Auth0 Community and have not found a solution.
[X] I agree to adhere to the Auth0 General Contribution Guidelines.
[X] I agree to uphold the Auth0 Code of Conduct.

SDK Version

8.3

PHP Version

PHP 8.2

Description

In https://github.com/auth0/auth0-PHP/issues/648 a BC break on v8.3 of the SDK gets mentioned, but these tickets automatically get locked for responses so that requires creating a new ticket.

Given that the above, of having to add cookieSecret while that was not needed in the SDK of v8.1.0 before (the one we currently have in production), can it please be considered to write a proper deprecation for this?

Suggestion:

let strategy fallback to none when no strategy is provided, and cookieSecret is set
let strategy fallback to management_api when no strategy is provided, and cookieSecret is not set
trigger_deprecation so implementation their test suites get informed about the deprecation that they have to explicitly configure the strategy

How can we reproduce this issue?

configure local implementation with Auth0 SDK 8.1
update SDK to 8.3 (or above)

https://github.com/auth0/auth0-PHP/compare/8.1.0...8.3.1#diff-80c341ea0969fc6f8f8e89f7d6968f64e1f3689daade522da96fbf60b5e472bdR581-R583

This snippet mentions the BC break that was made with a minor release.

evansims commented 1 year ago

Hi @rvanlaak, thanks for reaching out — I appreciate it!

The change you've mentioned corrected a configuration property (cookieSecret) not being treated as "required" when it should have been. The change brought functionality in line with what our documentation had already guided. We consider that change a bug fix. We don't feel it warrants introducing a new dependency for deprecation warnings or further changes at this time, but I thank you for the suggestion, and I sincerely apologize for any headaches this change may have caused you in upgrading.

rvanlaak commented 1 year ago

Get your point of view, but given it actually was not required before it do is a BC break.

The following snippet was valid and correctly working in 8.1 before, but with the 8.3 minor update it was not anymore:

$this->auth0Client = new Auth0([
    'domain' => $config->getDomain(),
    'clientId' => $config->getClientId(),
    'clientSecret' => $config->getClientSecret(),
]);
$this->auth0Client->configuration()->setManagementToken($this->getManagementToken());

The getManagementToken here is an implementation to fetch (and cache) the token from Auth0.

auth0 / auth0-PHP