Skip to content

Conversation

diabl0
Copy link
Contributor

@diabl0 diabl0 commented Mar 1, 2017

Follow-up to #192

Allows to use custom document classes. This will enable to have some business logic associated with particular document types and helps with using ArangoDB documents in different frameworks like Symfony.

Sample use case can be found in examples/customDocumentClass.php

@frankmayer frankmayer self-assigned this Mar 1, 2017
@frankmayer
Copy link
Contributor

Thanks, @diabl0 for your PR. It looks fine on a quick code review, besides the issue with the JsonSerializer. See my comment https://github.com/arangodb/arangodb-php/pull/208/files#r103711888.

@@ -19,7 +19,7 @@
* @package ArangoDBClient
* @since 0.2
*/
class Document
Copy link
Contributor

Choose a reason for hiding this comment

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

How did that get in? 😃

Implementation of JsonSerializable was rejected in PR #206 due to it adding a 70-100% overhead to object instantiation and handling (which does make a difference in a tight loop) and a way already exists to get the json-encoded data back (see #206 (comment)).

However, since your proposed changes allow subclassing, one has the choice to implement the JsonSerializer in a ny subclasses of Document, used. That gives people the choice to keep a more lean base class, avoiding said overhead.

@diabl0
Copy link
Contributor Author

diabl0 commented Mar 2, 2017

@frankmayer didn't noticed that discussion, so I removed \JsonSerializable from Document class itself and suggested using it in AbstractDocument in example implementation

@frankmayer frankmayer merged commit 63d437f into arangodb:devel Mar 2, 2017
@frankmayer
Copy link
Contributor

@diabl0 Great! Thank you again for your contribution.

@diabl0
Copy link
Contributor Author

diabl0 commented Mar 2, 2017

@frankmayer glad to help.

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

Successfully merging this pull request may close these issues.

4 participants