tal:omit-tag should have higher priority like condition or above

jah0wl commented 8 years ago

If you want to have a tag ommited, using the omit-tag directive, but, for example in this tag you want to access the same attribute you are checking to omit the tag, PHPTAL throws an error.

An example could be to display an image in a gallery and add a link to that image. Some images have link some others do not. You could wrap the img tag into an a tag, using tal:omit-tag to check if the $image/link exists. If it does, $image/link could be used to set the href attribute using the tal:attributes tag.

Here the code:

<a tal:omit-tag="not:exists:item/link" tal:attributes="href item/link">
    <img tal:attributes="src item/image" />
</a>

In its current implementation, this throws an error, because PHPTAL checks tal:attributes before it checks tal:omit-tag.

Potherca commented 8 years ago

I am not entirely sure this is indeed a bug.

When I run the example provided (with the code for Template1 below), I do get an error Array doesn't have key named 'link' but this also happens when I remove the tal:attributes property (see Template2).

This would indicate that the error does not come from the order the attributes are parsed in, but rather from the item/link not existing (instead of being NULL/false/empty/zero).

If I add an alternative nothing to the attributes (see Template3) the error message goes away and things work as expected. The same happens when item/link exists but is "falsey".

Instead of using nothing for the tal:omit-tag check, the combination of not:exists: can be used. Note, however, that nothing is still needed as a default for the tal:attributes attribute (see Template4).

Unless I am missing something (and based on these findings) I am inclined to think this is not a bug but expected behaviour: Trying to evaluate a key that does not exist triggers an error.

@Ocramius: Any thoughts on this?

The code used:

<?php

require_once '/path/to/PHPTAL/classes/PHPTAL.php';

$items = [
    [
        'link' => 'http://example.com/',
        'image' => 'example.png',
        'alt' => 'image with link',
    ],
    [
//        'link' => null,
        'image' => 'example.png',
        'alt' => 'image without link',
    ],
];

$sTemplate1 = <<<HTML
<div>
    <ul tal:repeat="item items">
        <li>
            <a tal:omit-tag="not:item/link"
               tal:attributes="href item/link"
            >
                <img tal:attributes="src item/image; alt item/alt" />
            </a>
        </li>
    </ul>
</div>
HTML;

$sTemplate2 = <<<HTML
<div>
    <ul tal:repeat="item items">
        <li>
            <a tal:omit-tag="not:item/link"

            >
                <img tal:attributes="src item/image; alt item/alt" />
            </a>
        </li>
    </ul>
</div>
HTML;

$sTemplate3 = <<<HTML
<div>
    <ul tal:repeat="item items">
        <li>
            <a tal:omit-tag="not:item/link | nothing"
               tal:attributes="href item/link | nothing"
            >
                <img tal:attributes="src item/image; alt item/alt" />
            </a>
        </li>
    </ul>
</div>
HTML;

$sTemplate4 = <<<HTML
<div>
    <ul tal:repeat="item items">
        <li>
            <a tal:omit-tag="not:exists:item/link"
               tal:attributes="href item/link | nothing"
            >
                <img tal:attributes="src item/image; alt item/alt" />
            </a>
        </li>
    </ul>
</div>
HTML;

$iCounter = 0;
while($iCounter < 4) {
    $iCounter++;

    printf('<hr/><h2>%s</h2>', $iCounter);

    $oTemplate = new PHPTAL();
    $sTemplate = 'sTemplate' . $iCounter;
    $oTemplate->setSource($$sTemplate);
    $oTemplate->set('items', $items);

    try {
        echo $oTemplate->execute();
    } catch (Exception $eAny){
        echo $eAny->getMessage();
    }
}

?>

jah0wl commented 8 years ago

First of all, you are right by using "not:item/link". I should use "not:exists:item/link".

But, if the point is to add the "| nothing" expression to all those uses of omit-tag accessing that same variable, why doesn't it occur to tal:condition expressions? If instead of having a basic TALES expression but a php: or string: expressions, how can I use "nothing" there?

I think it is a BUG, because I need to change all the behaviours of expressions like tal:attributes or tal:content with tal:omit-tag, and I don't need to do so for tal:condition, when they both do the same (removing the element or the tag if the condition doesn't match)...

And changing all those expressions because the omit-tag is not evaluated before those expressions is a pain in the ass.

Ocramius commented 8 years ago

In its current implementation, this throws an error, because PHPTAL checks tal:attributes before it checks tal:omit-tag.

Two things on this:

you cannot rely on the order of operations here. Also, if the order was guaranteed, it would still be subtle to remember it
tal:omit-tag is just a rendering concern. From my point of view, you should be able to render the tag anyway, and then tal just removes it depending on the outcome of the tal:omit-tag expression.

If I add an alternative nothing to the attributes (see Template3) the error message goes away and things work as expected. The same happens when item/link exists but is "falsey".

This sounds like the correct solution to me.

jah0wl commented 8 years ago

I add more:

You, of course, you have to rely on the order of operations. That's how we all work. I know that tal:condition is checked before anything else. It's not subtle to remember. Tal:condition and tal:omit-tag have the same use: One removes the entire tag and it contents and the other only the tag, but leaving the contents. In tal:condition, if you refer to any variable that is not set, but the condition is false, you have no problem, It doesn't happen with tal:omit-tag.
I disagree with the idea of tal:omit-tag is just a rendering concern. tal:condition is not? Or it renders the tag and its content or it doesn't. It's a waste of resource to calculate all expression just to not rendering them because of the tal:omit-tag.

From my point of view, this should be the priority order:

define
condition
repeat
content or replace
omit-tag
attributes

It's only not to calculate what affects directly to the tag, as attributes. The rest, PHPTAL should consider the tag as a tag-block, for repeating, replacing and content

Potherca commented 8 years ago

Instead of the responsibility lying with PHPTAL to know when an object has a certain property or not (and executing a command or not), it could just as easily be stated the it is the responsibility of the program/developer to ensure that a certain property is always available on a given object (but "falsey" if unavailable for use).

The order of priority is not candidate to change. It is the same as the original implementation in Zope.

As the omit-tag is the last on the list, that dictates which solutions are available to us.

That said, I agree that the behaviour is not entirely intuitive. If the only effect would be that the required fix is an improvement, not a bugfix, it could still be worth looking into...

If the fix doesn't interfere with other logic and does not require too much work, it could still be worth implementing. Especially in regards to your point in regards to php: and string: expressions.

I don't have time this week but I wouldn't mind looking into this at a later date.

Potherca commented 8 years ago

TL;DR

I've looked into this in more detail. I'm going to have to close this issue as a "won't fix".

The details

The doc-block of classes/PHPTAL/NamespaceAttribute.php references the Zope/TAL manual. It is (roughly) the same as what I linked to in my previous comment.

Order of Operations

When there is only one TAL statement per element, the order in which they are executed is simple. Starting with the root element, each element's statements are executed, then each of its child elements is visited, in order, to do the same.

Any combination of statements may appear on the same elements, except that the content and replace statements may not appear together.

When an element has multiple statements, they are executed in this order:

define

condition

repeat

content or replace

attributes

omit-tag

Since the on-error statement is only invoked when an error occurs, it does not appear in the list.

The reasoning behind this ordering goes like this: You often want to set up variables for use in other statements, so define comes first. The very next thing to do is decide whether this element will be included at all, so condition is next; since the condition may depend on variables you just set, it comes after define. It is valuable be able to replace various parts of an element with different values on each iteration of a repeat, so repeat is next. It makes no sense to replace attributes and then throw them away, so attributes is last. The remaining statements clash, because they each replace or edit the statement element.

If you want to override this ordering, you must do so by enclosing the element in another element, possibly div or span, and placing some of the statements on this new element.

In order to change or add more subtleties to the relevant logic in the code seems like more work than it is worth for the size of the change, especially as other means are available. (Like making sure a property is always present but falsey or adding a check in the template.

phptal / PHPTAL

tal:omit-tag should have higher priority like condition or above #45

TL;DR

The details