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

[ASK] Edge->getById #243

Open
lshaf opened this issue May 31, 2018 · 6 comments
Open

[ASK] Edge->getById #243

lshaf opened this issue May 31, 2018 · 6 comments

Comments

@lshaf
Copy link

lshaf commented May 31, 2018

when I use function getById, why Edge doesn't use documentClass just like document did? is there any reason? I want to know.

@lshaf lshaf changed the title [ASK] Edge [ASK] Edge->getById May 31, 2018
@jsteemann
Copy link
Contributor

jsteemann commented Jun 1, 2018

@lshaf : you mean like this?

index 3e1f0b5..a0e5eea 100644
--- a/lib/ArangoDBClient/EdgeHandler.php
+++ b/lib/ArangoDBClient/EdgeHandler.php
@@ -74,7 +74,9 @@ class EdgeHandler extends DocumentHandler
      */
     public function createFromArrayWithContext($data, $options)
     {
-        return Edge::createFromArray($data, $options);
+        $_documentClass = $this->_documentClass;
+
+        return $_documentClass::createFromArray($data, $options);
     }
 
 

That would make the getById method honor the documentClass specified.
As far as I can see, the EdgeHandler class does not take _documentClass into account anywhere. So probably some of its other methods need adjustment too.

According to git log, the document class support was contributed by @diabl0 in 2017.
@diabl0 : was there any specific reason to exclude the EdgeHandler from using document class?

@lshaf
Copy link
Author

lshaf commented Jun 4, 2018

@diabl0 do you mind to answer?

@jsteemann
Copy link
Contributor

Looking at this again, I think the main problem with using $_documentClass everywhere is that there may be different document classes for documents and edges.
For example, the following code from Cursor.php makes a distinction between documents and edges at the moment:

        $entry = [
            'vertices'    => [],
            'edges'       => [],
            'source'      => $_documentClass::createFromArray($data['source'], $this->_options),
            'destination' => $_documentClass::createFromArray($data['destination'], $this->_options),
        ];
        foreach ($data['vertices'] as $v) {
            $entry['vertices'][] = $_documentClass::createFromArray($v, $this->_options);
        }
        foreach ($data['edges'] as $v) {
            $entry['edges'][] = Edge::createFromArray($v, $this->_options);
        }

I think using $_documentClass::createFromArray also for the edges here will not be ideal.
But probably adding $_edgeClass, with a default to \ArangoDBClient\Edge and using that for creating edges would help.
What do you think? @lshaf @diabl0

@lshaf
Copy link
Author

lshaf commented Jul 27, 2018

it will be good and maybe you need to move setDocumentClass to Document and add setEdgeClass to Edge.

Just my idea.

@jsteemann
Copy link
Contributor

Example implementation in #251

@lshaf @diabl0 : can you please comment on it? Thanks!

@lshaf
Copy link
Author

lshaf commented Aug 6, 2018

Looks good but I found something look unnecessary.
What do you think? @jsteemann

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

No branches or pull requests

2 participants