Skip to content

fixed #126 relation querying in 0.6#127

Closed
luceos wants to merge 4 commits intorcrowe:0.6from
luceos:0.6
Closed

fixed #126 relation querying in 0.6#127
luceos wants to merge 4 commits intorcrowe:0.6from
luceos:0.6

Conversation

@luceos
Copy link

@luceos luceos commented Jul 15, 2014

#126 fixed resolving of attributes in templates concerning eloquent models and specifically their relations, whereas loading as method loads an querybuilder and loading as property loads a collection

… and specifically their relations, whereas loading as method loads an querybuilder and loading as property loads a collection
@luceos
Copy link
Author

luceos commented Jul 15, 2014

I forgot about the test by the way 😄

@barryvdh
Copy link
Collaborator

Do we really need reflection? Doesn't this hurt performance?
What about just checking if it's a model first, and then call $item->$object() for method calls, and $item->$object for the rest, so. I lay to what we do now, except that we also handle the method calls.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.13%) when pulling bf712e3 on Luceos:0.6 into 267131f on rcrowe:0.6.

@luceos
Copy link
Author

luceos commented Jul 15, 2014

Reason for reflection is to check for a static method. Eloquent models can't handle them for us and to twig there is no difference. However simple calling $object->$item will not call that static method, but will attempt to load a relation via the getAttribute method of the eloquent model, which will throw an exception.

Play around with the proposed code with the following scenarios:

{% for item in model.items %}
{% endfor %}
{% for item in model.items.paginate(10) %}
{%endfor %}
{% for item in model.items().paginate(10) %}
{%endfor %}
{{ model.staticMethod }}
{{ model.simpleMethod }}

Something like the above will prove the need to split. No idea whether the proposed check for static calls can be left out. The parent::getAttribute can probably handle it, but we're filtering on the eloquent model and thus not handling this via the parent method.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.24%) when pulling bb53727 on Luceos:0.6 into 267131f on rcrowe:0.6.

@luceos
Copy link
Author

luceos commented Jul 15, 2014

I'm rewriting to lose the reflection. Also seems like I opened up another bug somewhere. Murphy's jokes.

@luceos
Copy link
Author

luceos commented Jul 16, 2014

By using call_user_func_array on any possible method or static method, we can ensure a static check can be left out. Sadly my page won't render now when a QueryBuilder is returned due to memory exhaustion, a Collection is no issue though.

@barryvdh
Copy link
Collaborator

Can you provide you model and template? I can't really replicate this with the latest 0.6-dev.

I have this twig template:

<h2>model.items</h2>
{% for item in model.items  %}
    {{ item.id }}<br/>
{% endfor %}

<h2>model.items()</h2>
{% for item in model.items()  %}
    {{ item.id }}<br/>
{% endfor %}

<h2>model.items().get()</h2>
{% for item in model.items().get()  %}
    {{ item.id }}<br/>
{% endfor %}

<h2>model.items().paginate(2)</h2>
{% for item in model.items().paginate(2)  %}
    {{ item.id }}<br/>
{% endfor %}

And a model with this relation, that currently has 3 items:

public function items(){
    return $this->hasMany('MenuItem');
}

And this is my result, which seems correct (second returns a query, so shouldn't work.

model.items
1
2
3

model.items()

model.items().get()
1
2
3

model.items().paginate(2)
1
2

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ae02b4a on Luceos:0.6 into 267131f on rcrowe:0.6.

@luceos
Copy link
Author

luceos commented Jul 16, 2014

@barryvdh current code on 0.6-beta fails on static methods.

I just pushed a new version minifying everything to the base minimum.

@barryvdh
Copy link
Collaborator

Did you compare this with beta3 of the dev-0.6@dev version (the code your now editing)?

@luceos
Copy link
Author

luceos commented Jul 16, 2014

I'm not getting any results from the pagination at all with that the master 0.6-beta3 version. However I now do call the relation as a method and run pagination on that builder as well. Most likely this is not supported in that version?

@barryvdh
Copy link
Collaborator

I'm not sure about beta3, I tested with the latest dev version, and it worked there.

@luceos
Copy link
Author

luceos commented Jul 16, 2014

When using the latest branch 0.6 version any static method will run through the getAttribute method from the Eloquent model, where it will attempt to load the method as a relation, which fails on line 2379 of Eloquent\Model with an exception that the returned result is no relation.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.79%) when pulling 27adda2 on Luceos:0.6 into 267131f on rcrowe:0.6.

@rcrowe
Copy link
Owner

rcrowe commented Aug 13, 2014

See #126

@rcrowe rcrowe closed this Aug 13, 2014
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