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

Adding magic __call() #700

Closed
wants to merge 1 commit into from
Closed

Conversation

cassianotartari
Copy link

I'm using Elastica with Symfony and when I try to call for a key that has null as value inside Twig the Symfony throws:

Method "keyName" for object "Elastica\Result" does not exist in MyBundle:FolderName:myTemplate.html.twig at line X

Inside twig template I call like this:

{{ elasticaResultVariable.keyName }}

Adding this magic __call it works.

Maybe there is something better to do, so you can cancel this PR.

I'm using Elastica with Symfony and when I try to call for a key that has ``null`` as value the Symfony throws:
```
Method "keyName" for object "Elastica\Result" does not exist in MyBundle:FolderName:myTemplate.html.twig at line X
```

Inside twig template I call like this:
```twig
{{ elasticaResultVariable.keyName }}
```

Adding this magic ``__call`` it works.

Maybe there is something better to do, so you can cancel this PR.
@fprochazka
Copy link
Contributor

This is awful... retrieving data using magic __call ... why don't you pass the $result->getData() directly to the template, instead of the result object?

@cassianotartari
Copy link
Author

@fprochazka because I'm using Pagerfanta and I'm passing a Elastica\ResultSet to the template and running across the results with a loop.

Is there a better and clean way do to this? Just if I set in each iteration of the loop the getData value to a new twig variable...

@fprochazka
Copy link
Contributor

I just don't see how adding the magic call fixes it. There is already a __get, so where is the problem? Maybe try search for solution in the Twig?

@rmruano
Copy link
Contributor

rmruano commented Oct 9, 2014

@fprochazka It seems they're looking for a method, not for a property.

@cassianotartari Doesn't seem to be a Elastica related issue, and adding a __call magic method is not a proper solution. Perhaps you can create your own factory class which return an array of your custom Result objects (extending Elastica\Result and implementing the magic method) based on the Elastica\ResultSet. You can complicate it as much as you want / need.

Pseudocode:

myFactoryClass::createMyCustomResults(Elastica\ResultSet $resultSet) {
    $myResults = array()
    foreach $result inside $resultset
          $myResults[] = new myCustomResultObject( $result->getHit() )
    endforeach
    return $myResults
}

@cassianotartari
Copy link
Author

@fprochazka adding this magic method the error stops. The only thing that I've found in twig docs is this http://twig.sensiolabs.org/doc/recipes.html#using-dynamic-object-properties talking about the already implemented magic methods __isset and __get. But when the attribute has null as value it throws the error like I said.

Thanks @rmruano I'll study some different solution like yours

@fprochazka
Copy link
Contributor

@cassianotartari there is a difference between "an error stops" and "problem is solved".

@cassianotartari
Copy link
Author

@fprochazka ok if you want to read the "problem is solved", the problem is solved when I add this method, call {{ elasticaResultVariable.keyName }} and it returns nothing/null in my template.

P.S.: If you have something interesting to solve the issue in a standardized way please write it down otherwise I dispense comments.

@rmruano
Copy link
Contributor

rmruano commented Oct 9, 2014

@cassianotartari I can see what your problem is, Result::__isset() magic method is returning false for null values (as it should ever be), that causes your engine to avoid trying to get the property directly (which would trigger the __get to return the null value) and falling back to calling a method with the same name which doesn't exists.

Please notice that this is entirely an issue of your templating engine, which should probably just assume a null value if the getter method also doesn't exist instead of throwing errors. You can ask them to implement that behaviour (like @fprochazka suggested) or fall back to the solution I gave you. Implementing magic call methods is not a very good practice.

@cassianotartari
Copy link
Author

@rmruano thanks for the clarification!

@merk
Copy link
Collaborator

merk commented Oct 9, 2014

The correct method for accessing this information in twig is {{ result.data.key }} or {{ attribute(result.data, keyFromVariable) }}.

@merk
Copy link
Collaborator

merk commented Oct 9, 2014

Additionally, twig can be configured to ignore non existant variables which is talked about in the documentation (and even enabled in symfony2-standard's production mode)

@merk
Copy link
Collaborator

merk commented Oct 9, 2014

Sorry - I misunderstood the problem. The issue is twig will throw an exception on nonexistent properties by default, there are a few different ways of handling this, and my above solution won't work if the property is undefined.

{{ result.key | default('') }} will provide you with a default value if it is undefined,
{% if result.key is defined %}{{ result.key }} {% endif %} will only print the value if it is defined.

@cassianotartari
Copy link
Author

Thanks @merk! Your first comment solves the issue with Twig. The property is already defined, comes from elasticsearch mapping.

Another way that I thought was like I said before:

{% for result in resultSet %}
    {% set obj = result.data %}
    {{ obj.key }}
{% endfir %}

But again, the code would be more clean without this set or calling .data for each property.

@merk
Copy link
Collaborator

merk commented Oct 10, 2014

Your definition of cleanliness doesnt match mine. You want to iterate over data in an array from the result. Not a property of result.

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