Warning with cakephp3.5 about legacy-twig-view HOT 35 CLOSED

Ideally a "native" support for Cake blocks would be great.

Something like:

{% start 'header' %}
<h1>Title</h1>
{% end %}

{% append 'header' %}
<h2>Subtitle</h2>
{% end %}

{% fetch 'header' %}

from legacy-twig-view.

4.0.6 has been tagged with a fix for this thanks to @inoas 👍

from legacy-twig-view.

Another option I thought of is to implement __toString() on View, and return ''. This would solve the compatibility issue for both this and other plugins.

from legacy-twig-view.

The problem here is that CakePHPs minor release is non-BC.

@inoas You are dead-wrong. This is not a BC break from the framework perspective.
"Void to sth" is always fully BC.
If the plugin and Twig used the return value of void methods and by accident just worked so far, that is the plugin's problem, not CakePHP core's. Please understand this difference.

That said, the solution now provided seems to fix this in some way. So seems to be all good with the next patch release. Just be a bit more careful with your accusations, please :)

from legacy-twig-view.

The change here is from returning string to $this which is a return type change and that is non-BC, can you confirm this?

The change is from returning void to $this (see the code).

I suspect that you expected that assign() would have returned a block content. The behavior has not changed, only the return value has - from nothing to something.

Please, again, get your facts straight before you start telling people that they are wrong.

do you agree that we should not force people to upgrade their Twig Templates during a minor CakePHP release

Yes. I'm talking about a future plugin release.

IMO there needs to be some other way of manipulaitng Cake blocks. Currently the state changing functions (start(), assign(), append(), etc) are wrapped in echo blocks which isn't the good idea.

from legacy-twig-view.

@inoas But you complained about 3.5 in particular. But the only change there was cakephp/cakephp#11134 which is BC (void to $this is always BC as per BC guidelines).
Any other change regarding assign() seems to be in a very early 3.x version, though. So quite some years ago.

from legacy-twig-view.

@Orken #61 will fix your problem once merged and tagged.

#62 is the followup issue to track the correct implementation in future, for all those who are interested.

from legacy-twig-view.

Could you also include
/tmp/cache/twigView/93/9354f08c1f47e54248409d9f11b74ab4e9f9d332c0df7ef5d38a4a486cb660cb.php

from legacy-twig-view.

You'll find it attached here.

3d5e0211dc28c59bab413b8dac0ddbe4bc9521105d145d749578bb453c993008.zip

from legacy-twig-view.

Sorry, I have included the wrong file, here's the good one.

9354f08c1f47e54248409d9f11b74ab4e9f9d332c0df7ef5d38a4a486cb660cb.zip

from legacy-twig-view.

Some methods return $this since 3.5.

I've found a workaround for this, use none filter:

{{ _view.assign('title', __("I'm title"))|none }}

from legacy-twig-view.

What about

{% _view.assign('title', __("I'm title")) %} {# execute setter #} Edit: That's sadly not the way Twig works.
{{ _view.fetch('title') }} {# just echo #} Edit: fixed

from legacy-twig-view.

@inoas have you checked if your examples work, or maybe you are just guessing?

from legacy-twig-view.

Half guessing. I do only use a few parts of TwigView in my projects and I am not using _view.*

Trying:
{% _view.assign('title', __("I'm title")) %} {# execute setter #} <= Not a valid tag
{{ _view.get('title') }} {# just echo #} <= not an existing property
{{ _view.title }} {# this may work as well, try-and-see #} <= not an existing property

p.s.: I could not even find the docs for none https://twig.symfony.com/doc/2.x/filters/index.html

from legacy-twig-view.

Ok, so please don't mislead people :)

none filter is provided by this plugin.

from legacy-twig-view.

Ok, so please don't suggest to use dirty hacks :)

The problem here is that CakePHPs minor release is non-BC.

from legacy-twig-view.

As I said it is a workaround which actually works.

from legacy-twig-view.

It does not.

You still need {{ _view.fetch('title') }} as |none will stop returning $this but not output the title as before as CakePHP 3.5 broke that feature non-BC.

from legacy-twig-view.

I'll ask again: have you tried this or still guessing?

from legacy-twig-view.

Have you, I have.

from legacy-twig-view.

I use this workaround successfilly in my code.

Are we talking about the same?

{{ _view.assign('block', 'value')|none }}
{{ _view.fetch('block') }}

from legacy-twig-view.

Yes that will work, despite being a hack (running echo return, looking at WyriHaximus\TwigView\Lib\Twig\Extension\Strings). Without the fetch call it won't work.

Can you see a better way to get around the hack required because of assign/set returning $this in CakePHP 3.5? The only way I found is creating quite a bunch of Twig-Tags as far as I understand: https://twig.symfony.com/doc/2.x/advanced.html#tags as {% foo %} seems only to allow DSL constructs and no Twig Simple Functions.

from legacy-twig-view.

We should probably revert the changes in CakePHP then.

from legacy-twig-view.

Well 3.5 has been out for some time now and that could have impacts too(?) so my suggestion (disclaimer @robertpustulka I have tried it, right now ;-) would be, to overwrite assign() and __set() in class TwigView to not return anything?

// in TwigView:
    public function assign($name, $value)
    {
        $this->Blocks->set($name, $value);
    }

from legacy-twig-view.

@inoas That is another option as well. I can put together a pull request for that if it would be helpful.

from legacy-twig-view.

I'll wait for @WyriHaximus to chime in and if he concurs I will do that and hopefully also add working unit tests.

from legacy-twig-view.

@inoas This could work 👍

from legacy-twig-view.

Summary: We should either revert CakePHP 3.5 or fix TwigView 4.x & 5.x to make sure existing Twig templates run without touching them (production etc).

A quick copy&adapt. I am not sure we ll need/want all of them (consistency? would you set the template path within a twig-template, ever, etc)?

In class TwigView:

    /**
     * {@inheritDoc}
     * @return void
     */
    public function setTemplatePath($path)
    {
        parent::setTemplatePath($path);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function setLayoutPath($path)
    {
        parent::setLayoutPath($path);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function enableAutoLayout($enable = true)
    {
        parent::enableAutoLayout($enable);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function setTemplate($name)
    {
        parent::setTemplate($name);
    }

These for sure:

    /**
     * {@inheritDoc}
     * @return void
     */
    public function setTheme($theme)
    {
        parent::setTheme($theme);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function setLayout($name)
    {
        parent::setLayout($name);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function start($name)
    {
        parent::start($name);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function prepend($name, $value)
    {
        parent::prepend($name, $value);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function append($name, $value)
    {
        parent::append($name, $value);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function assign($name, $value)
    {
        parent::assign($name, $value);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function reset($name)
    {
        parent::reset($name);
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function end()
    {
        parent::end();
    }

    /**
     * {@inheritDoc}
     * @return void
     */
    public function extend($name)
    {
        parent::extend($name);
    }

Question: I assume about the same amount of functions that should go inside TwigView would otherwise need to become Twig Tags for TwigView + CakePHP 3.5 to run without changes on Templates?

from legacy-twig-view.

I am going to give this a try before hacking other ways.

from legacy-twig-view.

I gave it a try locally (in my SimpleTwigView ... but it should be the same).
Following works without being required to change any twig template code:

Within your class TwigView

    /**
     * {@inheritDoc}
     * @return string
     */
    public function assign($name, $value)
    {
        parent::assign($name, $value);

        return $value;
    }

    /**
     * {@inheritDoc}
     * @return string
     */
    public function __toString()
    {
        return '';
    }

Tested with

<h1>{{ _view.assign('title', __("I'm title")) }}</h1>
{{ _view.start('header') }}
<p>I'm header</p>
{{ _view.end() }}
{{ _view.start('footer') }}
<p>I'm footer</p>
{{ _view.end() }}
<p>I'm content</p>
{{ _view.fetch('header')|raw }}
{{ _view.fetch('footer')|raw }}

Output

<h1>I'm title</h1>
<p>I'm content</p>
<p>I'm header</p>
<p>I'm footer</p>

@WyriHaximus is this approach good with you?

Would any of you want to have any further setter methods to return their values for BC reasons?

from legacy-twig-view.

@inoas I'll test it and dive into this issue tomorrow 👍

from legacy-twig-view.

@inoas I was proposing to implement __toString() on CakePHP's view class.

from legacy-twig-view.

@markstory I am not sure that that is entirely good "implement __toString() on CakePHP's view class":

<h1>{{ _view.assign('title', __("I'm title")) }}</h1> would run without any warnings, but render an empty <h1/> and in less of a chance for the app-developer to catch the problem.

@dereuromark not sure if I am the one who is dead wrong, this time ;-):

1. If the change was from any-non-void to void; How would that be BC? Say a transformator suddenly storing the transformed value instead of returning it?

2. The change here is from returning string to $this which is a return type change and that is non-BC, can you confirm this?

Edit: assign() never returned something despite that the TwigView docs made me believe it would.

@robertpustulka yep, however:

1. do you agree that we should not force people to upgrade their Twig Templates during a minor CakePHP release, let alone they know about the problem. If we find a rather good compromise, It is bad enough that templates will break unless people will update/upgrade to the upcoming 4.x TwigView as well. To do that the solution above is fitting, or can you think about anything better?

2. After (1.) we can prep a PR to add Twig-Tags for start, append, prepend, assign, fetch, reset, and end and deprecate the old way to use them?

3. In a future TwigView release we can remove the old way of calling {{ _view.some_setter() }}?

from legacy-twig-view.

As assign() never returned something despite that the TwigView docs made me believe it would, implementing __toString() in CakePHP 3.5 like suggested will fix this problem.

I can take care and implement the tags for start, append, prepend, assign, fetch, reset, end (tags do not (necessarily) echo) on top of the next major TwigView release.

from legacy-twig-view.

Same/Similar Issue: cakephp/cakephp#11276 (comment)

from legacy-twig-view.