Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use 1.0.0 as the default statement version #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xabbuh
Copy link
Collaborator

@xabbuh xabbuh commented Mar 29, 2017

No description provided.

@xabbuh xabbuh force-pushed the default-statement-versions branch from d0aec9a to 58e40b2 Compare March 29, 2017 21:44
@Lctrs
Copy link
Contributor

Lctrs commented Mar 29, 2017

Why not do this during the serialization as done with the objectType of the Actor model ?

@xabbuh
Copy link
Collaborator Author

xabbuh commented Mar 30, 2017

The serialiser is also used by the client. If we implemented it the, the client would always send a statement containing a version.

@Lctrs
Copy link
Contributor

Lctrs commented Mar 30, 2017

Then, why not setting it at the storage level, before storing a statement ?

@xabbuh
Copy link
Collaborator Author

xabbuh commented Apr 3, 2017

We could, but that would be inconsistent with how we did handle other constraints till now (for example, we check for already existing statements inside the bundle but not in the storage layer).

@Lctrs
Copy link
Contributor

Lctrs commented Apr 3, 2017

This means we can have stored statements which lack a version, which is not fully compliant with the spec.

I'm not sure I'm okay with this. It can be hard then to manage statements at different version as the spec evolves.

@xabbuh
Copy link
Collaborator Author

xabbuh commented Apr 3, 2017

That's true. But then I suggest that we look at the bundle again and check which other logic should better be move to the storage layer too (and add tests for the things to the php-xapi/repository-api package.

@Lctrs
Copy link
Contributor

Lctrs commented Apr 4, 2017

For what it's currently implemented, I think we should move the check for existing statements inside the storeStatement method of the Statement repository.

With the current implementation, the library allow a behavior that is not compliant with the spec.

Looking at the LRS implementation from ADL, they do this check for both POST and PUT requests.

@Lctrs
Copy link
Contributor

Lctrs commented Apr 4, 2017

Plus, I just noticed that we are not currently checking for the existence of a voided Statement. Maybe we should add a method to the StatementRepositoryInterface that can retrieve both voided and non-voided statements ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants