search

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

Improve Deep Link ltiResourceLink model compliance #93

Closed alessandrodolci closed 1 year ago

alessandrodolci commented 1 year ago

Summary of Changes

As already stated in #92, the class that models Deep Linking resources of type ltiResourceLink is currently missing some properties mentioned in the specification, and misusing some others.

With this changes, I tried to fix the structure to match the standard, by introducing classes that map the missing properties and modifying the existing ones, while trying to maintain backwards compatibility. Since the presentation property seems to be non-standard, and is anyway superseded by window and iframe, it seemed reasonable to flag its accessors as deprecated, in order to try to discourage usage of it, without however introducing breaking changes to the library API.

I hope you'll find value in this pull request, of course I'm always available to further clarify on aspects you find difficult to figure out, or to modify parts that you do not find acceptable. I tried to stick to the coding standards I found to be used through the project as much as possible, but I guess some of my choices could be debatable.

I also would like to point out that there will continue to be room for further development, since other resource types are defined by the spec for Deep Linking response messages, and the library currently only supports the ltiResourceLink type.

Testing

I added unit tests trying to cover all the code changes I made, and I also took the opportunity to write some tests for methods defined inside the LtiDeepLink class, as they were mostly missing and I noticed a todo message was present.

Changes have also been tested by integrating the library inside an LTI Tool and trying them out by launching deep linking flows on a registered Moodle 4.1 Platform.

dbhynds commented 1 year ago

Sorry for the delay on this. It looks like there's an error with the tests and linters. I'm working on fixing this in a separate branch. You may need to merge main into this one once it's fixed. I'll keep you updated.

alessandrodolci commented 1 year ago

Hello @dbhynds, thank you for feedback. I've synced my branch with upstream and commited a fix to pass the new linting rules. Let me know if there's something else I can do.

Edit: I've just noticed that one of the checks is still failing, but everything seems fine when I run the linter locally. Could it be something on the CI environment?

dbhynds commented 1 year ago

I've run into this from time to time, if it's the issue I'm thinking it is. Try removing the .php_cs.cache and/or .php-cs-fixer.cache files from your local and re-running the linter.

Otherwise this is looking good.

dbhynds commented 1 year ago

@lin-brian-l tagging you on this PR in case I go on vacation before it can be merged. There's (what appears to be) just a single linting error preventing this from merging. I pushed up a branch testing these changes on our platform, and it passed: https://gitlab.com/packback/questions/-/jobs/4516883610, so we should be good there.

alessandrodolci commented 1 year ago

@dbhynds I see, thank you. I'm gonna try out what you suggest as soon as I get back to my pc.

alessandrodolci commented 1 year ago

I tried removing the cache files, but that didn't help.

The actual issue lay on the fact that I was still using version 1.2.0 of jubeki/laravel-code-style, as you're requiring version 2 of the dependency in logical or with version 1 in the project manifest file, and my previously generated lock file declared version 1, which was still satisfying your new constraint. Updating to version 2 of the dependency made the linting issue pop out correctly. You may consider adding the composer lock file to git, in order for everyone to be aligned and to avoid this kind of issues.

alessandrodolci commented 1 year ago

Thank you again for your effort. If I'll have the opportunity, I'll try to continue the work on the Deep Linking spec, adding implementations for the remaining content item types listed here.

Have a nice day, Alessandro