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

preferred label and eventcalendar #777

Merged
merged 9 commits into from
Jun 2, 2023

Conversation

thomas-topway-it
Copy link
Contributor

fixes #764

***attention! the proposed solution is a workaround for the use with SRF's ext.srf.api.query.js -> toList (a regex to match the printout label and custom label), however a preferred solution would be at SMW level trying to fix PrintRequest's Serializer -> doSerializeProp

* @param array $ask
* @return array
*/
private static function appendPreferredPropertyLabel( $printRequests, $ask ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use types in PHP and remove the doc block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tho you could keep the doc block and add more detailed type info such as PrintRequest[] or whatever the class name actually is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the param type
* @param PrintRequest[] $printRequests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it being imported, this references to SRF\PrintRequest, which does not exist.

You need to make sure the type is actually correct. Otherwise it does not help by providing autocompletion and error detection in the IDE. PhpStorm can autocomplete on the type in the comment and even import it automatically for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it should rather be @param SMW\Query\PrintRequest[] $printRequests ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would resolve to SRF\SMW\Query\PrintRequest[]. You need a backslash at the front: \SMW\Query\PrintRequest[]

Though better is to import the name with an import statement, so you do not have the FQN in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it. Is it preferred with "use" also if the class object is not directly called ? (in this case is rather returned by $queryResult->getPrintRequests() )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Import statements are cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the import statement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing, @JeroenDeDauw !

@JeroenDeDauw JeroenDeDauw merged commit 3b52776 into SemanticMediaWiki:master Jun 2, 2023
4 checks passed
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.

[eventcalendar] fails when properties have a preferred label
3 participants