[BUG]: TagFactory __invoke and Html\Helper\Meta,Link,Script,Style. Store reset at each call

[yannux]

Describe the bug
When calling multiple times the Meta helper, previous stored values disappear.

To Reproduce

//IndexController.php
$this->tag->meta()->addName('robots', 'index, nofollow');

//index.phtml
<?= $this->tag->meta()->addName('author', 'Me'); ?>

Only output

<meta name="robots" content="index,nofollow">

Another quick example

var_dump($this->tag->meta()->addName('test1', 'test1')->addName('test2', 'test2')->__toString());
var_dump($this->tag->meta()->addName('test3', 'test3')->__toString());

Expected behavior
A clear and concise description of what you expected to happen.

//IndexController.php
$this->tag->meta()->addName('robots', 'index, nofollow');

//index.phtml
<?= $this->tag->meta()->addName('author', 'Me'); ?>

Must output

<meta name="robots" content="index,nofollow">
<meta name="author" content="me">

Details

• Phalcon version: 5.3.0
• PHP Version: 8.1.23
• Operating System: DEbian
• Installation type: pecl
• Server: Apache

Additional context

Each time you call $this->tag->meta() AbstractSeries::__invoke reset the store

[yannux]

It's seems that AbastractSeries is only used by Meta,Script,Style

Maybe just remove this->store = []; inside AbstractSeries::__invoke ?

[niden]

I am not sure if this is a bug to be honest.

Doing this:

$this
    ->tag
    ->meta()
        ->addName('robots', 'index, nofollow')
        ->addName('author', 'Me')
;

will produce

    <meta name="robots" content="index, nofollow">
    <meta name="author" content="Me">

If you change the example to this:

// IndexController.php
$this->tag->meta()->addName('robots', 'index, nofollow');

// index.phtml
<?php $this->tag->meta()->addName('author', 'Me'); ?>
<?= $this->tag->meta(); ?>

then it should work as expected. The chaining makes the __toString() called first since you use <?= and the contents get cleared, and then the assignment comes with addName. I think that is what is happening there.

A test that I wrote is as follows

        $escaper = new Escaper();
        $meta    = new Meta($escaper);
        $meta
            ->addName('robots', 'index, nofollow')
            ->addName('author', 'Me')
        ;

        $expected = '    <meta name="robots" content="index, nofollow">'
            . PHP_EOL
            . '    <meta name="author" content="Me">'
            . PHP_EOL;
        $actual   = (string) $meta;
        $I->assertSame($expected, $actual);

        $meta = new Meta($escaper);
        $meta->addName('robots', 'index, nofollow');

        $expected = '    <meta name="robots" content="index, nofollow">'
            . PHP_EOL
            . '    <meta name="author" content="Me">'
            . PHP_EOL;
        $actual   = (string) $meta->addName('author', 'Me');
        $I->assertSame($expected, $actual);

As you can see it works with either setting the addName further down.

I am wondering, can you try something like this and let me know?

//IndexController.php
$this->tag->meta()->addName('robots', 'index, nofollow');

//index.phtml
<?= ($this->tag->meta()->addName('author', 'Me')); ?>

[s-ohnishi]

I tried.

// IndexController.php
$this->tag->meta()->addName('robots', 'index, nofollow');

// index.phtml
<!-- start -->
<?php $this->tag->meta()->addName('author', 'Me'); ?>
<?= $this->tag->meta(); ?>
<!-- end -->

The results are below.

<!-- start -->
<!-- end -->

Currently, I think it is possible to obtain the expected output with the following.

// index.phtml
<!-- start -->
<?= $this->tag->newInstance('meta'); ?>
<?= $this->tag->meta()->addName('author', 'Me'); ?>
<!-- end -->

In this case, it will be as follows.

<!-- start -->
    <meta name="robots" content="index, nofollow">
    <meta name="author" content="Me">
<!-- end -->

Although the method name is "new"Instance, it returns the registered "old"Instance.

PHP 8.1.20
Phalcon 5.3.1

[s-ohnishi]

Since you can only have one TagFactory for each service (tag), you cannot write multiple tags at the same time unless you retain the instance of the service you created first.

If you use $this->tag->meta(), a new instance will be returned, so if you do the following, the viewport will disappear.

class IndexController extends \Phalcon\Mvc\Controller
{
     public function beforeExecuteRoute($dispatcher)
     {
         $this->tag->meta()->addName('viewport', 'idth=device-width, initial-scale=1.0');
     }
     public function indexAction()
     {
         $this->tag->meta()->addName('robots', 'index, nofollow');
     }

Hasn't this been pointed out as an odd specification (probably a bug)?

If you want to use it like this, you need to do the following.

class IndexController extends \Phalcon\Mvc\Controller
{
     protected $meta;
     public function beforeExecuteRoute($dispatcher): void
     {
         $this->meta = $this->tag->meta()->addName('viewport', 'idth=device-width, initial-scale=1.0');
     }
     public function indexAction()
     {
         $this->meta->addName('robots', 'index, nofollow');
     }

Honestly, I think it's very inconvenient.

[yannux]

Yes it's not conveniant.

I try another workaround,

//BaseController (used for common code for others controller
$this->view->meta = $this->tag->meta();


//IndexController extends BaseController
$this->view->meta->addName('description', 'foo');

//index.phtml  works with this->view ok $meta
<?= $this->view->meta; ?> //works
<?= $this->view->meta->addName('foo', 'bar'); ?> //works

[yannux]

Hasn't this been pointed out as an odd specification (probably a bug)?

Don't know, Niden said on discord :

Meta should be doing the same thing as title because it is the same code that handles either of those helpers.

(https://discord.com/channels/310910488152375297/310910488152375297/1104470868131852392)

So if it's intended to work like Title, we can expect to be able to do $this->tag->meta() on different file without loosing data ?

[s-ohnishi]

Similar to title, which is output only once, meta, style, script, etc., which output multiple tags, return instances registered in TagFactory, so their usage is the same.
However, as pointed out by yannux, __invoke() uses this->store = [];, so the contents are cleared every time it is called.

There is no problem with clearing it when you intend, but since it is not common, I think it is acceptable to call it in a special way.
I think adding something will be used a lot.
Currently, it is quite troublesome to have to call tag->newInstance('meta')->addName() to a registered "old" instance and be careful not to clear its contents.

Conversely, you may want to output the same tag with different content regardless of what is registered in store.
now

<?= $this->tag->meta()->addName('something', 'foo'); ?>

is very intuitive and easy to use, but it destroys registered information, so I don't think there is any other way than to write the HTML directly instead of using a helper.
If newInstance() really returned a new instance, we wouldn't have to do that.

[s-ohnishi]

Looking at the manual, it seems that CSS and JavaScript are based on the use of assets.
I don't think there is any alternative for meta.
They may choose to extend the assets rather than change the obscure specification of the AbstractSeries class.
I think it's a bug, so I'm hoping that the twisted specifications will be fixed to a straightforward one.

[s-ohnishi]

@yannux

So if it's intended to work like Title, we can expect to be able to do $this->tag->meta() on different file without loosing data ?

Currently, I think this is possible by using $this->tag->newInstance('meta').
It's not "new" at all, so I'm not convinced by the name and specifications.

[yannux]

@s-ohnishi

So if it's intended to work like Title, we can expect to be able to do $this->tag->meta() on different file without loosing data ?

Currently, I think this is possible by using $this->tag->newInstance('meta'). It's not "new" at all, so I'm not convinced by the name and specifications.

Yes I'm ok with that.

So currently I'm using this solution :

//BaseController (used for common code for others controller
$this->view->meta = $this->tag->meta();


//IndexController extends BaseController
$this->view->meta->addName('description', 'foo');

//index.phtml  works with this->view ok $meta
<?= $this->view->meta; ?> //works
<?= $this->view->meta->addName('foo', 'bar'); ?> //works

I found it cleaner / more readable.

[s-ohnishi]

@yannux
I'm glad you found your solution with the current Phalcon.

[niden]

@yannux What is the status of this?

I saw you made some changes as a workaround by assigning the meta() to a variable. Did any of the suggestions I posted work?

Can you try this:

//IndexController extends BaseController
$this->tag->meta()->addName('description', 'foo');

//index.phtml
<?= $this->tag->meta()->addName('foo', 'bar'); ?>
<?= $this->tag->meta(); ?>

[yannux]

Hi @niden sorry I was focus in test and exchange with s-ohnishi and forget to answer you :(

The result is

 <!-- start $this->tag->meta()->addName('foo', 'bar') -->
     <meta name="foo" content="bar">
 <!-- end $this->tag->meta(); -->

Adding $this->meta() to a view variable is a simple/good workaround own our side.

[niden]

This is so weird. I will do some more investigation on this. You should be seeing both meta tags. Sigh

[niden]

Thank you for testing

[yannux]

Each time you call $this->tag->meta() AbstractSeries::__invoke reset the store

maybe because of this?

[niden]

Most likely yes. That is where I was leaning

[niden]

Initially I was thinking of introducing a ->reset() method which would clear the store. It looks that we need that.

[s-ohnishi]

Initially I was thinking of introducing a ->reset() method which would clear the store. It looks that we need that.

I think you need a method to remove elements in the store more than a method to clear the store.
However, to do this, you will need a search method that uses partial string matching or regular expressions to identify "the element".

[s-ohnishi]

There were other things that were needed.
The loading order of both style sheets and JavaScript has meaning.
For some reason, someone may need to load an element they want to add before a specific element.
spl_autoload_register() does not allow us to control the order in detail, but it is possible to insert at the beginning as well as append at the end.
We can also remove specific autoload functions with spl_autoload_unregister().
I think we need at least the same level of control.

[christoferd]

Just some ideas...

$this->tag->meta() is probably creating a new instance every time.

Is that method supposed to check if an instance already exists and is attached?

$this->tag does this always reference the same instance?

[s-ohnishi]

$this->tag->meta() is probably creating a new instance every time.

It does not create a new instance.
However, as already pointed out, when returning the instance, the registered data is cleared before returning it, so it is almost the same as a new instance.

Is that method supposed to check if an instance already exists and is attached?

yes. Returns an instance that already exists (registered as a service).

$this->tag does this always reference the same instance?

yes. I think it's returning an instance of the same TagFactory.

I think the first step to solving the problem is to avoid clearing the registered data when returning the instance.

[niden]

Resolved in #16455

Added a reset() method and removed the clearing of the internal store on __invoke().

At the moment manipulation of entries (remove for instance) is not feasible since they do not have a unique name. That could be a NFR for the future.