-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update data models #111
base: master
Are you sure you want to change the base?
Update data models #111
Conversation
3e6dd3e
to
1efb644
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't have a class called Class in PHP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
DateTime::toISO8601() is also not a method, its just DateTime::ISO8601();
Erroneous line:
$sessionSeries->getStartDate()->toISO8601() |
Docs ref: https://www.php.net/manual/en/class.datetimeinterface.php#datetimeinterface.constants.iso8601
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange as the class called Class existed previously: https://github.com/openactive/models-php/blob/f78e51db0d329636a504ecc13409d8af7aab4713/src/Models/SchemaOrg/Class.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the autoloader never loaded it because it was never used before?
<?php
// Try to make a class called Class
class Class
{
public function __construct()
{
echo 'hi';
}
}
$myClass = new Class();
running this script gives: Parse error: syntax error, unexpected token "class", expecting identifier in class-test.php on line 4
expected output is hi
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense! Given the error was already here I wonder if we merge this PR and then solve this issue separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 5.6 test is failing for a different reason (this one: #111 (comment)) to the 7.4 test.
But it seams reasonable to move the class thing to a new issue
I've created #112 which should allow this to pass? |
dcde052
to
04a8a7b
Compare
Update PHP data models to the latest version based on the OpenActive Vocabulary (codified by the Data Models), Test Interface and Beta Namespace.