packbackbooks / lti-1-3-php-library

A library used for building IMS-certified LTI 1.3 tool providers in PHP.
Apache License 2.0
39 stars 25 forks source link

Breaking changes introduced by patch version (v5.2.2) #77

Closed emmachughes closed 1 year ago

emmachughes commented 1 year ago

1a58cbeae47d32067de75f4ea47d5043a8d1c4bf and 8bee17978970cfda7d25f68a69ca9bcd03a1727c introduced changes to Packback\Lti1p3\Interfaces\IMessageValidator that break both existing usage of validators and implementations of this interface.

Example of broken usage, caused by validate() now returning void instead of false:

use Packback\Lti1p3\MessageValidators\ResourceMessageValidator;

$validator = new ResourceMessageValidator();

if (!$validator->validate($jwtBody)) {
    // After the change, this code path is always followed.
    handleTheFailure();
}

Existing implementations of IMessageValidator will have been broken after a new method were added, return types added, and instance methods changed to be static.

Finally, I believe the change to static methods to be misguided, as implementations of these methods can no longer make use of services injected via constructor. While this is fine with Laravel, it is an issue when using frameworks without global service locators, such as Symfony.

dbhynds commented 1 year ago

Interesting. I originally intended the validators to only be used internally by this package itself. I wasn't expecting anyone use the package to need to interact with them. Can you tell me more about how you're using validators or custom implementations of them? It might be worth implementing those in the package itself.

emmachughes commented 1 year ago

I originally intended the validators to only be used internally by this package itself. I wasn't expecting anyone use the package to need to interact with them.

As it turns out, the original developer on our project had copied LTIMessageLaunch into the source code and modified it instead of implementing the interfaces to integrate with the rest of the application. So this doesn't seem to be a big deal after all, then. We have no custom validators.

Can IMessageValidator at least be marked as /** @internal */? It does reside along interfaces that are expected to be implemented, after all.

dbhynds commented 1 year ago

Excellent suggestion. I'll do that. Thank you.