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

Type Product is adding @id instead of identification #156

Closed
JohJohan opened this issue Dec 24, 2020 · 4 comments · Fixed by #159
Closed

Type Product is adding @id instead of identification #156

JohJohan opened this issue Dec 24, 2020 · 4 comments · Fixed by #159

Comments

@JohJohan
Copy link
Contributor

When generating Product with an id it will generate a @id instead of identification. https://schema.org/Product shows https://schema.org/identifier

This was probably introduced by #102

@Gummibeer
Copy link
Collaborator

Gummibeer commented Dec 24, 2020

The identifier property and its sub-properties are primarily useful in cases where the content is expressed as a textual string. Increasingly there are canonical URL/URI representations for each of these. All schema.org syntaxes already have built-in representation for URIs and URLs, e.g. in Microdata 'itemid', in RDFa 1.1, 'resource', in JSON-LD, '@id'. Generally it is preferable to use these unless there is a specific requirement to explicitly state the kind of identifier, or to provide additional / alternative identifiers (e.g., DOIs). Such requirements are common e.g. for scientific dataset description.
-- https://schema.org/docs/datamodel.html#identifierBg

After reading it again I think that a middle solution would be right.
@id is the correct key for simple string identifiers but typed identifiers should remain identifier.
I won't work on this until mid January. But I would welcome a PR that fixes this.

I wish you a merry Christmas! ⛄🎄🦌

@JohJohan
Copy link
Contributor Author

Okay i would like to pick this up. I have an idea to create a interface class TypedIdentifierInterface that way i can check if i need to add @id or identifier is that a okay solution? And is there a way for me to find out what are simple string identifiers and what are tped identifiers?

@Gummibeer
Copy link
Collaborator

Hey,
we already have everything - only the snippet that replaces the identifier with @id has to be conditional.

protected function serializeIdentifier()
{
if (isset($this['identifier'])) {
$this->setProperty('@id', $this['identifier']);
unset($this['identifier']);
}
}

It should check if $this['identifier'] is a string or an object - you could check for Type interface.
Following the schema.org docs you can use an URL (which is string) or text or an instance of PropertyValue which we already have as an available type.
https://github.com/spatie/schema-org/blob/master/src/PropertyValue.php

JohJohan added a commit to JohJohan/schema-org that referenced this issue Dec 26, 2020
spatie#156  - render identifier instead of @id for typed identification
JohJohan added a commit to JohJohan/schema-org that referenced this issue Dec 26, 2020
JohJohan added a commit to JohJohan/schema-org that referenced this issue Dec 26, 2020
spatie#156 - Update pull request number and add extra check
JohJohan added a commit to JohJohan/schema-org that referenced this issue Dec 26, 2020
spatie#156 - Update pull request number and add extra check


spatie#156 - update pull request
@JohJohan
Copy link
Contributor Author

Hello @Gummibeer,

Thank you for helping out. I managed to make a pull request #157.

Let me know if i missed something.

Gummibeer added a commit that referenced this issue Jan 2, 2021
#156  - render identifier instead of @id for typed identification
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 a pull request may close this issue.

2 participants