Skip to content

Conversation

MLKiiwy
Copy link
Contributor

@MLKiiwy MLKiiwy commented Jun 18, 2015

For issue : #79

Add support of custom identifier for doctrine (other than Id)

  • auto detect custom identifier via DoctrineIdentifierLoader (if primary key is simple)
  • default id value set in AttributeMetadata
  • identifier property is now like other attribute of class and have is own AttributeMetadata (required to make it writable in certain case)
  • can set writable class identifier
  • add TF with CustomIdentifierDummy / CustomWritableIdentifierDummy
  • add documentation

The possibility to add an annotation to set our custom idenfier manually without doctrine was removed from this PR and moved in another future PR. Because it imply other changes ... in DataProvider fro example (cant use Doctrine getReference ... DataProvider is really doctrine linked)

@MLKiiwy MLKiiwy changed the title #79 custom identifier [WIP] #79 custom identifier Jun 18, 2015
@@ -97,6 +105,7 @@ class AttributeMetadata implements AttributeMetadataInterface
public function __construct($name)
{
$this->name = $name;
$this->isIdentifier = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't it be part of the constructor (with a default value) ? Even if it's not the case right now, I'd expect this attribute metadata to be immutable as much as possible.

@MLKiiwy
Copy link
Contributor Author

MLKiiwy commented Jun 19, 2015

Another question is posed by these Dev.
Actually we don't integrate the Id attribute in serialization process :

{
  "@context": "http://schema.org",
  "@type": "Country",
  "@id": "/countries/en",
  "name": "Angleterre"
}

I think it's an error, @id point to an IRI an must be present : OK
But an IRI != Id, and in many case we want also to return the id if present. (main reason : how in my sample, in an json-ld client I determinate that the @id refer to a field wich is not "id"

IMO, don't treat the attribute id differently than other attribute and user can set with a "@group" if he want to be serialized or not.

@dunglas
Copy link
Member

dunglas commented Jun 19, 2015

I think the internal (database) id should never be exposed directly and must be replaced by the public external identifier (the IRI). It's the motivation behind the current behavior.

@dunglas
Copy link
Member

dunglas commented Jun 19, 2015

About your use case: the JSON-LD should never know anything from server internals. It doesn't have to deal with the internal database id, it's the server responsibility.

@MLKiiwy
Copy link
Contributor Author

MLKiiwy commented Jun 19, 2015

Yes you're right but in my sample (and actually in my dev) I need to be able to set the identifier because it's not automatically determinate by the server.

The issue is the normalizer must have option to not ignore the identifier if it's needed on POST / PUT request.

Actually I set ignoring identifier on JsonLd/Serializer/ItemNormalizer.php but on denormalizer so we can set it on POST / PUT request.

@sroze
Copy link
Contributor

sroze commented Jun 19, 2015

@MLKiiwy It's maybe because you've design issues in your code. IMHO the Domain Objects (in that case the API output/input) mustn't care about the storage layer. The id is just here because you're using a relational database that put us on the way of using serials (or auto_increment for MySQL) that are not related at all with your business.

@MLKiiwy
Copy link
Contributor Author

MLKiiwy commented Jun 26, 2015

Still in WIP.

I add two tests : custom_identifier.feature and custom_writable_identifier.feature
And I have to add the test "custom_identifier_by_annotation.feature"

I think it cover all the needs, what do you think ?
Are you agree with these tests ?

MLKiiwy added 4 commits July 3, 2015 11:36
# The first commit's message is:
79-custom-identifier : add identify class identifier from doctrine

# This is the 2nd commit message:

#79-custom-identifier : wip

# This is the 3rd commit message:

#79-custom-identifier : wip

# This is the 4th commit message:

#79-custom-identifier : add : full tests cases

# This is the 5th commit message:

#79-custom-identifier : fix : config

# This is the 6th commit message:

#79-custom-identifier : wip

# This is the 7th commit message:

#79-custom-identifier : wip
@MLKiiwy MLKiiwy changed the title [WIP] #79 custom identifier #79 custom identifier Jul 3, 2015
@MLKiiwy
Copy link
Contributor Author

MLKiiwy commented Jul 3, 2015

@dunglas here is my last version !

Actually we identify the entity identifier from Doctrine Identifier.
We don't support entity resource with multiple identifier.

The entity identifier is never returned like other properties : it's a part of the URI contained in @id field.
Copy link
Member

Choose a reason for hiding this comment

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

Ca you put @id into backticks ``` and replace URI by IRI for coherence?

dunglas added a commit that referenced this pull request Jul 7, 2015
@dunglas dunglas merged commit 2d60104 into api-platform:master Jul 7, 2015
@dunglas
Copy link
Member

dunglas commented Jul 7, 2015

Thank you @MLKiiwy

leesiongchan pushed a commit to leesiongchan/api-platform-core that referenced this pull request Mar 6, 2018
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.

4 participants