Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with layouts renderer and sections => merge sections in one view #2491

Closed
RedskyThirty opened this issue Jan 16, 2020 · 15 comments · Fixed by #2502
Closed

Issue with layouts renderer and sections => merge sections in one view #2491

RedskyThirty opened this issue Jan 16, 2020 · 15 comments · Fixed by #2502
Milestone

Comments

@RedskyThirty
Copy link

RedskyThirty commented Jan 16, 2020

Hello,

It seems that there is a problem with the layouts and I think that this is due to "ob_start / ob_flush / ..." in the renderer.
It will be easier to explain my problem with an example...

public function send(): string
{
    $email = view('email');
   
    // Send email script here...

    return view('confirmation');
} 

View "email" extends a layout "x" with a section called "body".
View "confirmation" extends another layout "y" with a section called "body".

Sometimes (but not always), "view('confirmation')" displays the "body" section of "email" and "confirmation". The renderer merges both sections even if the layout is not the same.
This is really annoying. For the moment, I try to prefix all my sections in each layouts but it would be better if I didn't have to do this kind of things.

@lonnieezell lonnieezell added this to the 4.0.0 milestone Jan 17, 2020
@najdanovicivan
Copy link
Contributor

najdanovicivan commented Jan 18, 2020

@RedskyThirty Can you provide view files you're using? Views are rendered using the sharedRenderer service. I'm noticing some issues with properties $sections and $layout not being reset after outputting the view.

I'm mostly interested which name you're using for section.

@RedskyThirty
Copy link
Author

RedskyThirty commented Jan 20, 2020

@najdanovicivan I'm sorry, I cannot share my real files but here is what they look like:

Layout 01

Some HTML here...
<?= $this->renderSection('body') ?>
Some HTML here...

View 01

<?= $this->extend('layouts/layout_01') ?>
<?= $this->section('body') ?>
Some HTML here...
<?= $this->endSection() ?>

Layout 02

Some HTML here (different from layout 01)...
<?= $this->renderSection('body') ?>
Some HTML here (different from layout 01)...

View 02

<?= $this->extend('layouts/layout_02') ?>
<?= $this->section('body') ?>
Some HTML here...
<?= $this->endSection() ?>

@RedskyThirty
Copy link
Author

FYI, I renamed "body" by something like "l1_body" and "l2_body" to prevent the issue.

@najdanovicivan
Copy link
Contributor

Yes. Exactly what I'was taking about. As sections are not being reset on the end of rendering having sections with the same name in multiple layouts can cause issues.

@RedskyThirty
Copy link
Author

Yes, but am I wrong if I say that for the moment the framework has no method to finalize a "renderSection()"?

@najdanovicivan
Copy link
Contributor

I've submitted PR for this one. I've only added one line in the code. Can you manually paste

// Reset sections
$this->sections = [];

After line 249 in system/View/View.php to see if it fixes the issue for you ?

@MGatner
Copy link
Member

MGatner commented Jan 20, 2020

We’ll have to wait on the test outcome but this looks like a good and isolated change so I’m glad to merge shortly.

@MGatner
Copy link
Member

MGatner commented Jan 20, 2020

Oops, GitHub auto-closed. @RedskyThirty can you confirm if this is resolved?

@RedskyThirty
Copy link
Author

I don't have time to test now but I suppose it's ok ;-)

@najdanovicivan
Copy link
Contributor

@MGatner auto close happens because I put Fixes prefix in PR message

Fixes #2491

So once PR is merged the Issue gets closed automatically. I'm gonna send future PRs with Reference only

@MGatner
Copy link
Member

MGatner commented Jan 23, 2020

This was causing other issues that we don't have tests in place for; I had to revert the change for now until we can look into it.

@MGatner MGatner reopened this Jan 23, 2020
@najdanovicivan
Copy link
Contributor

@MGatner can you provide me a sample where the issue happens so that I can create test and then check the issue in more detail?

@MGatner
Copy link
Member

MGatner commented Jan 26, 2020

Sure. It happened on this project: https://github.com/MGatner/nexus

Notice that the layout has its own sub-calls to views, which I think is where the issue is. When the parser clears the sections those nested calls don’t resolve.

@RedskyThirty
Copy link
Author

Here you can find a simple demo of the issue ;-)
https://www.dropbox.com/sh/mxdpz6fd35txrpw/AACWbBPE4VlUENOjueImTIhJa?dl=0

@najdanovicivan
Copy link
Contributor

@MGatner and @RedskyThirty I've submitted new PR which takes a different approach on this one. Additionally I've included the unit test for the mentioned issue. it will be great if you can do some testing on your end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants