Bug when pushing to sections inside of fetched templates

odahcam commented 1 year ago

Imagine de following file structure.

One layout.phtml:

<html>
<head>
    <title>Document</title>

    <?php $this->section('custom-head') ?>
</head>
<body>
    <?php $this->content() ?>

    <?= $this->section('custom-footer', $this->fetch('footer-default')) ?>
</body>
</html>

One details.phtml page:

<?php $this->layout('layout.phtml') ?>

<h1>My Details page</h1>

<?php foreach ($items as $item) ?>
    <?= $this->fetch('item.phtml', ['item' => $item]) ?>
<?php endforeach ?>

One item.phtml page:

<?php $this->push('custom-head') ?>
<script>
    globalApiHelper.sendUserRenderedDetailsItem(itemPrice);
</script>
<?php $this->end() ?>

<h2>My item <?= $item->name ?></h1>

And finally

<?php
$renderedView = $platesEngine->render('details.phtml', ['items' => $items]);
echo $renderedView;

The push inside the fetched template (item.phtml) will never appear in the final $renderedView string.

ragboyjr commented 1 year ago

Good find.

My gut tells me that this should be using the parent section content when pushing or not or define its own. I think this should be doable to implement.

odahcam commented 1 year ago

Hey glad to see you're back!

I did some debugging in the past, but it's been too long, but I want to make some context for others reading this.

We have the fetch method and it just renders the child template (ie item.phtml) and takes the string output.

https://github.com/thephpleague/plates/blob/ec07462f53d38dd4a3323e288c41bffe86e9098e/src/Template/Template.php#L308-L317

The child template (.phtml) should be able to register the sections in the parent scope, details.phtml because since it was the target to ->render(...) Plates is creating a Template object which we're refering to as the scope and fetch method called is actually from it. But the fetch is actually using the engine render method which AFAIK will create a new Template object for the child items.phtml.

Given that I think a possible solution would be to code the fetch method something like this to have access to the child's metadata:

     public function fetch($name, array $data = array()) 
     { 
          $template = $this->engine->make($name); 
          $this->sections = array_merge($this->sections, $template->sections);
          // here we could grab more info from the child
          return $template->render($data);
     }

I'm assuming a high risk here of talking something completely nonsense hahaha, but this seems to make sense.

odahcam commented 1 year ago

Yeah, my idea above would never work, but I made some tests and implemented some working code in #306. There's a section mode issue though, described in the PR, that would be very nice if you could give your opinion.

odahcam commented 1 year ago

I think I covered everything I wanted at this point, it would be nice to have your input on the PR, thanks!

basteyy commented 1 year ago

And I thought I would be the only one with the use case ;) Thanks for the efforts so far. I like the input https://github.com/thephpleague/plates/pull/306#issuecomment-1400413897 from ragboyjr.

This is a (quick and dirty) fix (based on extensions) for my use case, where I want to push some script data to the layout:

// Register Extension
// file: e.g. index.php (somewhere near the engine definition)
$engine->loadExtension(new class implements ExtensionInterface {
        private string $content = '';
        public function register(Engine $engine)
        {
            $engine->registerFunction('addToSection', function($content) {
                $this->content .= ' ' . $content;
            });
            $engine->registerFunction('getSection', function () {
                return $this->content;
            });
        }
    });

// Add some data
// file home.php
$this->addToSection(<<<EOD
<script>
alert("Hello World");
</script>
EOD);

// Output somewhere in the layout (or any other template)
// file: layout.php
echo $this->getSection();

odahcam commented 1 year ago

@basteyy ohhh so that's why there are extensions like that! I wasn't getting the point of using something like it but now it's clear. Did any of you check my latest changes in the PR? It's actually looking good now, with clear APIs and automated tests. I really want some input to have this in the next Plates version as soon as possible.

charlesgetup commented 1 year ago

Hi guys,

Since this issue is still open, here is my solution. Just a thought for you guys to have a look.

In my situation, I have

admin.php (Admin layout)
users.php (User page which inherits the admin layout)
pagination.php (page partial which only holds the user table pagination and it is used by users.php)

And I have the following push function in pagination.php.

<?php $this->push('scripts') ?>
    <script src="/assets/pagination.js"></script>
<?php $this->end() ?>

So if I use insert/fetch function in users.php, the pagination.js is not loaded by users.php. I think that is the timing issue. When the partial (pagination.php) is rendered, the user page (users.php) has not rendered yet. So the pagination.js could not be rendered to layout's scripts section.

My fix is **include** the partial, not render it immediately. Let the partial render together with the User page (users.php). Then the problem has been solved.

Solution:

In users.php, write,

<?php **include** $this->getPath('admin/partials/pagination'); ?>

The getPath is a helper function which gets the template file path. In this way, I have no need to change the core code.

One disadvantage is adding variable(s) to the nested template. For example, in my User page, I have a variable called $userPaginationLimit. And in the pagination template, I want to use it as $limit. If I use insert/fetch function, I could write it like, $this->insert('admin/partials/pagination', array('limit' => $userPaginationLimit));. But in my solution, I have to write $limit = $userPaginationLimit; include $this->getPath('admin/partials/pagination');. So to make my code cleaner, I have to avoid this variable naming inconsistent situation.

charlesgetup commented 1 year ago

I spent another day on this issue and I believe that this is a design "issue". The "issue" happens at the render function in template.php file.

https://github.com/thephpleague/plates/blob/ec07462f53d38dd4a3323e288c41bffe86e9098e/src/Template/Template.php#L158-L188

I will continue using my example in the above comment here to explain my findings.

When using the insert/fetch function in a partial, it eventually calls this render function. From the render function, we can see, line #174, it requires layoutName to stack the sections. But in a partial, there is no layout referenced like below. Then take a closer look at the render function, it uses the "ob_*" functions to display the nested templates from inside out. So the partial will be rendered first. That means when the partial is first rendered, there is no way for it to have a layout. That is why the insert/fetch` function in a partial won't work.

<?php $this->layout('template', ['title' => 'Users']) ?>

My thought is adding the above line of code to the partial, then the partial has a layout name, and it could push/unshift code the the parent page sections. But this also won't work. No matter which template is used as layout in a partial, it will be rendered together with the partial, this may duplicate some of the page content, and all the variables passed to that layout need to pass to partial, too. And this will block the partial to be reused by other pages.

So to fix this "issue", I think we have to re-design the render function and find a proper way to pass the layout to the partial or render the partial in the right time. And I failed to make this change yesterday. This could be a big move and affect everything in the project. Finally, I have to come back to my solution in the above comment.

When I have a second thought of the include line of code, I found that it won't work when the partial needs to be included multiple times. In my example, if I want to include the pagination twice for users and user roles, I have to write the following code and the variables with same name will override each other.

...
$limit = $userLimit;
$total = $userAmount;
include $this->getPath('admin/partials/pagination');
...
$limit = $userRoleLimit;
$total = $userRoleAmount;
include $this->getPath('admin/partials/pagination');

In this comment, I made an improvement of the solution. Make it not that dirty and fix the variable same name issue.

The improved solution has two parts.

1, A new helper function. It will be used to replace the "insert/fetch" function. Here is the helper I wrote, a simplified version. You could add your logic in it.

function includePartial($name, $variables = array())
{
    // Extract the variables
    extract($variables);

    // Start output buffering
    ob_start();

    // Include the template file.
    // The "getPath" function is another help function which get template file path by its name.
    include $this->getPath($name);

    // This is about 2nd part, run the "push/unshift" function in partial when parent page is rendered.
    if(function_exists('loadDependencies')){
        loadDependencies($this->template); // You may pass some more variables to this function
    }

    // End buffering and render partial contents
    $output = ob_get_clean();

    echo $output;
}

2, Implement "loadDependencies" function in partial. In my example, on pagination.php file, the function is implemented like below.

<?php
function loadDependencies ($template) {
    $template->push('scripts');
?>
    <script src="/assets/js/pagination.js"></script>
<?php
    $template->end();
}
?>

Then in the parent page, the user.php file, I include the partial like below. And this will look nicer in some point.

<?php
    $this->includePartial('admin/partials/pagination', array('limit' => $userLimit, 'total' => $userAmount));
?>

I know this solution is not perfect, and if I have some better idea, I will make comment again (if this issue is still open, :smiley:).

Conclusion:

I like the ob_* functions way to render the nested page content. Very efficient. So if we don't call the template functions, e.g. push/unshift, within the template which has no layout, it runs perfectly. I think that is the design. We should use the template functions in a template with a layout. The only confusion is this is not mentioned in the document. I hope the author could explain this more clearly in the document inheritance page.

A very nice project, great work. I will keep using it and thanks the author for sharing. :thumbsup:

thephpleague / plates

Bug when pushing to sections inside of fetched templates #305