cms icon indicating copy to clipboard operation
cms copied to clipboard

Cascade method getViewData should only return the data from the current view and not all of it.

Open Bernier154 opened this issue 1 year ago • 4 comments

Bug description

I am currently creating a tag which takes all the param from the view and merge them to act like the AttributesBag from blade.

For this, i was using $this->context->view as a reference to the param of the current view. But i soon realized that if i didn't put the param class in each of my block using this tag, i would get a leak from a block that used the class param.

Example:

{{ partial:layout/block_1 class="cool_class" }} 
{{ partial:layout/block_2}} // if I check $this->contect->view here, it will have the class=>cool_class param

So i went out of my way by using the "view" property from the RuntimeParser to execute the Cascade::instance()->getViewData() method.

It works, but i get the same result.

I checked the method and i feel something is wrong with it. It gets the data from all views each time, and overwrite (merge) the current's view data, which feel unituitive because it's not only the view data, it's ALL the data.

    public function getViewData($view)
    {
        $all = $this->get('views') ?? [];

        return collect($all)
            ->reverse()
            ->reduce(function ($carry, $data) {
                return $carry->merge($data);
            }, collect())
            ->merge($all[$view])
            ->all();
    }

I look through the code and it's not used a lot so maybe it's an oversight.

For the time being, i'll use Cascade::instance()->get('views) and get the array i need. ButI felt it needed to be revised.

How to reproduce

  1. Create a couple partial and use them in succession in antlers.
  2. Create a tag that dumps $this->context->get('view')
  3. Watch as the params mixes the more the parser goes.

Logs

No response

Environment

Statamic
Addons: 2
Sites: 2 (Français, Anglais)
Stache Watcher: Enabled
Static Caching: Disabled
Version: 5.23.0 PRO

Statamic Addons
codems/core: dev-master
pixelastronauts/statamic-autocomplete-address-field: dev-main

Installation

Fresh statamic/statamic site via CLI

Additional details

No response

Bernier154 avatar Aug 27 '24 02:08 Bernier154

I ignored both above..

If you are interested about what i wanted to do, this is how I got the view param:

class Attributes extends Tags
{

    /**
     *  A bit hacky...
     */
    private function getViewName()
    {
        $reflectionProperty = new \ReflectionProperty(RuntimeParser::class, 'view');
        $reflectionProperty->setAccessible(true);
        return $reflectionProperty->getValue($this->parser);
    }

    private function getParamsFromCascade()
    {
        return Arr::get(
            Cascade::instance()->get('views', []),
            $this->getViewName(),
            []
        );
    }
    /* [...] */
}

Bernier154 avatar Aug 27 '24 03:08 Bernier154

For this, i was using $this->context->view as a reference to the param of the current view. But i soon realized that if i didn't put the param class in each of my block using this tag, i would get a leak from a block that used the class param.

Just to clarify, you're saying that variables from previous "replicator sets" are leaking into the cascade? Are you able to ptovide an example?

duncanmcclean avatar Aug 27 '24 07:08 duncanmcclean

Here is the quickest way to reproduce it:

  • Fresh statamic install
  • make a "debug" tag
    • image
  • Create some partials and insert the created tag in each of them
    • image
    • image
  • In the home view, use the partials and add some params to them
    • image
  • Serve and watch as the blocks get the class param from the first block
    • image

Bernier154 avatar Aug 27 '24 12:08 Bernier154

Thanks - I see it now.

By the way. you don't need to build a custom tag for debugging, you can just do {{ view | dump }}.

duncanmcclean avatar Aug 30 '24 10:08 duncanmcclean

As mentioned in #11178 , if fixing this is not so simple because of retro compatibility issues, it may be possible to add a new "self" (or another name) variable available inside partial, storing only values from that template... ( ie the same than "view" var but without the leaked values.. ) . What do you think?

dadaxr avatar Nov 29 '24 10:11 dadaxr

@dadaxr

I'm not sure because it affected the normal use aswell..

It affect repeated views especially. In my example, I found out the bug with my "block builder" (bard field with sets) where i had an advanced parameter class. This parameter after multiple time the same row rendered was leaking in subsequent blocks if the next block didn'nt hav a value to override it.

My solution was like you said, I scoped the bad field with "block", so i can use the "block:var" notation.

But this solution doesn't correct the behavior with view level parameters.

As for your problem, I don't think it would affect you implementation of "allExperts" (correct me) in the parent view. I don't consider data that was set above the view "leaked", I more refering to data that was set in an equal or further depth on a view :

  • mainview
    • view2
      • view 4
    • view3 (Should not have params set in view 2 or 4, unless it was a store initialized in mainview )

Your point is valid tho and there is a way to get the data from the template only:

If you check my above solution to my problem, I have rigged myself a function to get the view data only. It feels hacky because it is a private property that i forced public, but it works for me.

I ignored both above..

If you are interested about what i wanted to do, this is how I got the view param:

class Attributes extends Tags
{

    /**
     *  A bit hacky...
     */
    private function getViewName()
    {
        $reflectionProperty = new \ReflectionProperty(RuntimeParser::class, 'view');
        $reflectionProperty->setAccessible(true);
        return $reflectionProperty->getValue($this->parser);
    }

    private function getParamsFromCascade()
    {
        return Arr::get(
            Cascade::instance()->get('views', []),
            $this->getViewName(),
            []
        );
    }
    /* [...] */
}

This function only get the parameter set in the current view, so it works like the "SELF" variable proposed.

I still didn't take a look at solution for my problem because i made this temporary solution and sometime temporary become permanent untill it breaks 😅.

Bernier154 avatar Nov 29 '24 14:11 Bernier154