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

A "AttributeNotFoundExceptionHandler" interface would be nice for custom Filters. #223

Open
electrotype opened this issue Aug 27, 2016 · 10 comments

Comments

@electrotype
Copy link
Contributor

electrotype commented Aug 27, 2016

Currently, if I understand correctly, only the DefaultFilter filter will be called if an expression contains a not found attribute.

if (filter instanceof DefaultFilter) {
    try {
        input = getLeftExpression().evaluate(self, context);
    } catch (AttributeNotFoundException ex) {
        input = null;
    }
} else {
    input = getLeftExpression().evaluate(self, context);
}

It would be nice if there was an interface our custom filters could implement so they are applied even if an attribute is not found in the expression! My current workaround is to make my custom filter extend DefaultFilter, but an interface would be way cleaner.

@electrotype electrotype changed the title A "NullHandler" interface would be nice for custom Filters. A "AttributeNotFoundExceptionHandler" interface would be nice for custom Filters. Aug 27, 2016
@mbosecke
Copy link
Collaborator

Can you provide more information about your custom filter? I suspect that this is too niche of a problem that any solution is probably not worth the overhead.

@electrotype
Copy link
Contributor Author

In the framework I work on, I use Pebble to output informations if fields from a submitted Form contain errors.

For example, If "myForm.user.company.title" is invalid, I want to ouput an error message. But it is possible that "myForm.user.company" doesn't exist yet... In that case, I simply don't want to output anything. Note that I still want to use strictVariables everywhere in the application.

Using my workaround, I do something like :
{{myForm.user.company.title | fieldErrors}}

Since the fieldErrors Filter extends DefaultFilter, the Java code is called even if "myForm.user.company" doesn't exist yet, and I then simply output an empty String. If this field exist, I can validate it an output an error if required.

If I try to do something similar but using a function, for example :

{{fieldErrors(myForm.user.company.title)}}

then an exception is thrown when "myForm.user.company" doesn't exist.

In other words, I need the myForm.user.company.title expression to be evaluated, and my Java code to always be called even if one property doesn't exist. Is there another way to achieve that than my workaround?

I can submit a Pull Request for that, it's trivial to implement.

@mbosecke
Copy link
Collaborator

So, essentially you want to continue using strictVariables but in this particular instance you do not want the AttributeNotFoundException to be thrown.

I'm reluctant to allow this exception to be suppressed by arbitrary filters because it defeats the purpose of using strictVariables in the first place and it makes it harder to read a template when it's not obvious which filters suppress the exception and which ones don't. In fact, I don't even like that the DefaultFilter suppresses this exception.

My preference is to maintain status quo for now, but I'll keep this task open so that it can be revisited for the next major version of Pebble when we don't mind breaking backwards compatibility. At that time, I would want the solution to be crystal clear when reading the template that strictVariables has been temporarily turned off. Perhaps a dedicated filter for that reason: {{ user.company.title | strictVariables(false) | fieldErrors }}.

Some possible workarounds that you can implement with the current system:

  • Extend DefaultFilter like you mentioned
  • Use the DefaultFilter for the sake of suppressing the exception prior to feeding the input into your own filter. I.e.: {{ myForm.user.company.title | default(null) | fieldErrors }}. Looks awkward and not obvious what the intent is.
  • Use ternary expressions: {{ (myForm.user.company == null? null : myForm.user.company.title) | fieldErrors }}. This is probably the "cleanest" solution, although ternary expressions are verbose.

@mbosecke mbosecke added the 3.0 label Sep 22, 2016
@electrotype
Copy link
Contributor Author

electrotype commented Sep 22, 2016

Thanks for that reply. I understand your concerns, but I don't see how it would break any backwards compatibility or be confusing. You simply do:

if (filter instanceof AttributeNotFoundExceptionHandler) {

Instead of
if (filter instanceof DefaultFilter) {

and you make DefaultFilter the only one to implement that interface. Then developers have the freedom to implement the interface for there custom filters, if it's required.

But at least there are a couple of workarounds, so it ok for me! Thanks for your help.

@mbosecke
Copy link
Collaborator

The "confusing" problem with your proposed solution is when you read {{ user.company.title | myCustomFilter }} and you know that strictVariables is true, you have no idea whether or not it's null-safe until you dig open the source code for myCustomFilter. This is too big of a problem to go forward with that solution and I suspect that any other solution will require breaking backwards compatibility with the DefaultFilter.

@electrotype
Copy link
Contributor Author

I see... At least, don't make DefaultFilter final please! ;-)

@nikku
Copy link

nikku commented Oct 1, 2016

There is another option to consider: Why not add a syntax that allows users to safely query for null values, even if strictVariables is true?

Example

Let's assume the user attribute is either set or not.

The expression {% if user != null %}...{% endif %} will fail in strict mode.

If I am absolutely aware of the fact that the user may be optional, I could make that explicit, i.e. via {% if user? != null %}...{% endif %} to relax the strictVariables setting.

The filter mentioned above would simply reduce to the following then:

{{ myForm.user.company.title? | fieldErrors}} 

@electrotype
Copy link
Contributor Author

electrotype commented Oct 1, 2016

I still think that libraries and frameworks should ideally have the possibility to create functions and filters that are able to manage non existing attributes, without a special syntax... And even when strictVariables is on.

I currently have my hands deep into form validation, for the framework I work on, and I must say that without that "extending the DefaultFilter" hack, Pebble would probably be too limiting for what I need to do.

Let me give you more details about that already discussed "validation" use case...

Displaying an HTML form would look like this in my framework (simplified example) :

<form method="post">
    <input type="text" 
           class="{{validation.myForm.user.company.title | validationClass}}" 
           name="myForm.user.company.title" 
           value="{{myForm.user.company.title | fieldValue}}">
    {{validation.myForm.user.company.title | validationMessages}}
    <button type="submit">Submit</button>
</form>
  • I'm not fan of frameworks that hide the HTML generation by providing some "magic" tag/function which output the entire Form by itself! That's why I give the possibility to manually add :
    • The value of the field itself (using the fieldValue filter). That outputs an empty string if the value doesn't exist).
    • The css class for the input, depending of the validation result (using the validationClass filter)
    • The validation messages associated to a particular field (using the validationMessages filter), if any.
  • Plain Maps are used to access both the fields values and the validation results. This makes it very easy for a developer to use any templating engine (they all handle plain Maps) :
    • The value of the company title is taken from a plain Map : myForm.user.company.title.
    • The potential validation messages are also inside a plain Map, but a different one : validation.myForm.user.company.title.

I don't want to say to the developers : "You have to make sure that there are validation messages associated to a field before trying to output them.", that would be very ugly in my opinion... The {{validation.myForm.user.company.title | validationMessages}} way (using the "extending the DefaultFilter" hack) is not bad in that regard : it is straighforward and the developer doesn't have to worry about the fact that there may not be any validation messages for some fields.

Frameworks such as Spring MVC (with JSP for example) provide complex tags which handle such validation output, form:errors for example. The developers don't need to check if there are errors or not to use this tag...

In other words, I still think that preventing third-parties to provide functions and filters that handle non existing attributes is somewhat limiting.


That said, I see another option though : If the "expression parsing" algorithm Pebble uses was available using an API, then everything could work too. For example, if there was this method available :

EvaluationContext#evaluate(String expression)

Then we could write a function (or a filter) which would looks like : {{validationMessages("validation.myForm.user.company.title")}} (notice the quotes!)

And the function/filter could evaluate this expression by itself, using the evaluate(...) method AND it could handle non existing values, if required.

Sorry for the wall of text! :-)

@electrotype
Copy link
Contributor Author

electrotype commented Nov 27, 2016

I now have a live example of what I'm talking about. Have a look at the first filter, validationMessages(), for example.

I would have prefered to use functions there, but since there is no way to make them "attribute does not exist" tolerant, I used filters with the "extends DefaultFilter" hack [code].

I want the users to be able to have strictVariables on in their application. But the first time the form is displayed (not submited), the validation root variable doesn't exist, and throwing an exception here is not acceptable.

Anyway, it's just to show why a AttributeNotFoundExceptionHandler interface (or something similar) would be beneficial in some cases, imo.

@ebussieres
Copy link
Member

Related to #245

@ebussieres ebussieres added this to the 3.0.0 milestone May 5, 2018
@ebussieres ebussieres removed the 3.0 label Jul 13, 2018
@ebussieres ebussieres removed this from the 3.0.0 milestone Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants