statamic / cms

The core Laravel CMS Composer package
https://statamic.com
Other
4.13k stars 540 forks source link

Session tag wildcard method could fails when a conflicting variable exists in the current context #10643

Open dadaxr opened 3 months ago

dadaxr commented 3 months ago

Bug description

AFAIK Ihe variables created inside {{ nocache tag }} are not accessible to other nocachetag... So my idea was to use the session to share existing data between two {{ nocache tag }} but it seems that fetching data from session can behave inconsistently according to the variable defined in the current scope (context)

ie :

{{ nocache }}
  {{ variableToShare = { heavy:processing } }}
  {{ session:set :variableToShare="variableToShare" }}
  ... some template stuff using this variableToShare
{{ /nocache }}

... other template stuff 
{{ nocache }}
 {{ myRetrievedVariabled = {session:variableToShare} }} => myRetrievedVariabled is empty, it does not work
 {{ myRetrievedVariabled = {session:value key="variableToShare} }} => myRetrievedVariabled is filled, it's ok.

   ... some other template stuff using this variableToShare
{{ /nocache }}

So put simply I could say : if a variable named "xxx" exist in the current context, the tag {{ session:xxx }} would not work as intended (documented).

Moreover, the {{ session:value key="xxx" }} tag method is not documented (source diving helps ;))

How to reproduce

{{ my_var = 'value' }} {{ session:set my_var="another value" }}

{{ session:my_var }} {# output nothing : it's broken }} {{ session:value key="my_var" }} {# output "another value" : it's working }}

Logs

No response

Environment

Environment
Laravel Version: 10.48.13
PHP Version: 8.2.7
Composer Version: 2.6.2
Environment: local
Debug Mode: ENABLED
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: CACHED

Drivers
Broadcasting: log
Cache: file
Database: mysql
Logs: stack / single
Mail: smtp
Queue: sync
Session: file

Statamic
Addons: 4
Sites: 1
Stache Watcher: Disabled
Static Caching: Disabled
Version: 5.20.0 PRO

Installation

Fresh statamic/statamic site via CLI

Additional details

No response

dadaxr commented 3 months ago

Also ... It is worth mentioning that trying to access session data using {{ session.variableToShare }} (using a dot) does not work, as "session" is not a variable accessible from the cascade ... unlike this commit title suggests : https://github.com/statamic/cms/commit/1b2567aed202363909a8394d0f4fc479c33cb752

duncanmcclean commented 3 months ago

This seems similar to #10612, where the value of the item in context is being passed to the wildcard tag, instead of the plain key. 🤔

You shouldn't need to use {{ session:value )), it's used under the hood when you do (( session:my_var )) which is why it's not documented. It's a good workaround for now though.

Also ... It is worth mentioning that trying to access session data using {{ session.variableToShare }}

The . syntax only applies to arrays of values, not tags. A lot has changed since 2020 when that commit happened.

jasonvarga commented 3 months ago

This is going to be confusing, so bear with me. 😅

In this bit:

{{ variableToShare = { heavy:processing } }}
{{ session:set :variableToShare="variableToShare" }}

Pretend for a moment that {heavy:processing} returns a string of "heavy".

In session:set you are putting a key named literally variableToShare into the session with a value of heavy.

Then over here:

 {{ myRetrievedVariabled = {session:variableToShare} }} => myRetrievedVariabled is empty, it does not work

The shorthand session tag will try to grab the key from the context. So, you have a variable named variableToShare with a value of heavy sitting in the context, so it will try to grab the heavy key from the session, which doesn't exist.

This behavior where we grab stuff from the context is a bit weird in hindsight, but to "fix" it now would be a breaking change. We can probably get rid of it in v6 though.

Then, over here:

 {{ myRetrievedVariabled = {session:value key="variableToShare"} }} => myRetrievedVariabled is filled, it's ok.

You are getting a key of literally variableToShare from the session, which does exist and has a value of heavy. So it works fine.


What you probably should do is this:

  1. Figure out what you want the session variable to be named, and what the value should be.
    • Name it foo, for example.
    • The value should be what the heavy tag returns.
    • {{ variableToShare = {heavy:processing} }}
      {{ session:set :foo="variableToShare" }}
  2. Retrieve it either way, both will work now because you're not naming the session key the same as the temporary variable.
    • {{ session:foo }}
    • {{ session:value key="foo" }}

All together it could look like this:

{{ nocache }}
    {{ variableToShare = {heavy:processing} }}
    {{ session:set :foo="variableToShare" }}
{{ /nocache }}

{{ nocache }}
    {{ myRetrievedValue = {session:foo} }}
    or 
    {{ myRetrievedValue = {session:value key="foo"} }}
{{ /nocache }}
dadaxr commented 3 months ago

It indeed works, but looks more like a workarround to me ;)

Would it still be a breaking change if in the "wildcard" method we check first in the session, and if no result, we fallback to lookup in the "context" ? ie something like that ( caution : "hardcore coding" bellow ) ?

public function wildcard($tag)
    {
        $result = $this->value();

        if ( is_null($result) ) {
             $key = $this->context->value($tag, $tag);

              $key = str_replace(':', '.', $key);

              $result = session()->get($key, $this->params->get('default'));
        }
        return $result;
    }

public function value()
    {
        $key = $this->params->get('key');

        $key = str_replace(':', '.', $key);

        return session()->get($key, $this->params->get('default'));
    }

If not... maybe add a "warning" in the session doc specifying "carefull, if a variable in the current scope has the same name, it would be returned instead" ?

anyway ... Thanks for your previous answer :)

duncanmcclean commented 3 months ago

Yes, it is indeed a workaround.

We can fix the core issue in v6, which is coming next year. I think the workaround Jason suggested is fine until then (considering how many people have ran into the issue in the years it's worked like this).