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

Added sections inheritance of fetched templates #306

Open
wants to merge 5 commits into
base: v3
Choose a base branch
from

Conversation

odahcam
Copy link
Contributor

@odahcam odahcam commented Jan 21, 2023

This PR partially addresses the case described in #305 , it still lacks considering the section mode of each of the appended sections where you can append, prepend or replace the sections entirely. Unfortunately we don't current store how the sections where filled in the template so it is currently impossible to know that during the inheritance in the parent template. My current best idea for that is to create yet a new property called $sectionsAddedMode so we can track per section name the mode used during the push/unshift/replace, e.g. $sectionsAddedMode = [ 'head' => 1, 'appbar' => 1, 'footerScripts' => 2 ].

section mode handling is still missing though
src/Template/Template.php Outdated Show resolved Hide resolved
@odahcam
Copy link
Contributor Author

odahcam commented Jan 22, 2023

My latest commit now addresses my comments.

@ragboyjr
Copy link
Contributor

Let's add a test to highlight the bug, and then also we'd need to support this in the insert method as well.

Another solution which might work fine is:

  1. creating a new mutable class called TemplateSections which you can push/unshift/add/replace/etc template section content.
  2. Each Template object has an instance of TemplateSections and optionally can accept a TemplateSections in the constructor.
  3. when creating partials via insert or fetch, we can pass in the current templates TemplateSections instance to the child which would cause the push/unshift/etc methods to interact with the same instance of template sections which I think is definitely a bit more of the expectation.

Your solution isn't bad, but having one mutable class that defines the API clearly might simplify the solution.

@odahcam
Copy link
Contributor Author

odahcam commented Jan 24, 2023

@ragboyjr I agree with you, I don't know if I can find time any sooner to do that though. Maybe next weekend.

@odahcam
Copy link
Contributor Author

odahcam commented Feb 4, 2023

@ragboyjr my latest commit addresses the need for a TemplateSections API and defines the base for the implementation of the other ideas you gave. I'd like you to take a look at this initial stage and help me define the next steps.

@odahcam
Copy link
Contributor Author

odahcam commented Mar 7, 2023

I've added more template section tests.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest limit it to .idea

@@ -173,7 +174,9 @@ public function render(array $data = array())

if (isset($this->layoutName)) {
$layout = $this->engine->make($this->layoutName);
$layout->sections = array_merge($this->sections, array('content' => $content));
$layout->sections->merge($this->sections);
$contentSectionName = 'content';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to use a attribute to declare it. In case someone want to change it.

Copy link

@basteyy basteyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the changes, and it seems to me that this works fine.

$this->mode = $mode;

// if this template doesn't have that section, so we just add it.
if (empty($this->content)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like !isset instead of empty. Some time ago I read that this would be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's because empty also checks if the value represents an empty value like "".

@Abdillah
Copy link

Abdillah commented Mar 26, 2023

Unfortunately we don't current store how the sections where filled in the template so it is currently impossible to know that during the inheritance in the parent template.

Let the template knows the parent and reuse the parent's section may solve this issue. I think that's more proper point of view on this problem-space.

I mean,

diff --git a/src/Template/Template.php b/src/Template/Template.php
index 21c66f0..b096633 100644
--- a/src/Template/Template.php
+++ b/src/Template/Template.php
@@ -40,6 +40,8 @@ class Template
      */
     protected $data = array();
 
+    protected $parent;
+
     /**
      * An array of section content.
      *
@@ -77,11 +79,12 @@ class Template
      * @param Engine $engine
      * @param string $name
      */
-    public function __construct(Engine $engine, $name)
+    public function __construct(Engine $engine, $name, $parent = null)
     {
         $this->engine = $engine;
+        $this->parent = $parent;
         $this->name = new Name($engine, $name);
-        $this->sections = new TemplateSectionCollection();
+        $this->sections = $this->parent ? $this->parent->sections : new TemplateSectionCollection();
 
         $this->data($this->engine->getData($name));
     }

@Abdillah
Copy link

Abdillah commented Mar 27, 2023

Ah, I see some complication when using layout.
By the way, I have implemented the merging of multiple section modes based on this PR:
#310

@odahcam
Copy link
Contributor Author

odahcam commented May 15, 2023

I think referencing the same sections as the parent template does is exactly one of the things we want to avoid. IN my understanding the Render should handle reading all sections and put them all together accordingly to whats registered in each template.

@odahcam
Copy link
Contributor Author

odahcam commented May 15, 2023

@Abdillah could you add the scenario for the issues you saw with using layout so I can make a test for it?

@Abdillah
Copy link

I'm on vacation, I'll be back tomorrow.

@odahcam
Copy link
Contributor Author

odahcam commented Jul 5, 2023

ping @Abdillah

@Abdillah
Copy link

Abdillah commented Jul 6, 2023

Hello, I'm sorry for not responding soon.

I've written some of my though in this comment (quoted below). I expect Plates to overwrite section when multiple child template using ->start() ... ->end() and append the ->push() ... ->end() content after the rewrite.

I'm thinking of grouping REWRITE before any mutation. That is consistent with what template system should do in my mind.

layout.php

<?php $this->section('javascript') ?>

page.php

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

<?php $this->start('javascript') ?>
  <script src="base-page-script-very-light-weight.js"></script>
<?php $this->end() ?>

<?= $this->fetch('common-component-a') ?>
<?= $this->fetch('heavyweight-component-b') ?>

heavyweight-component-b.php

<?php $this->start('javascript') ?>
  <script src="base-page-script-support-heavy-weight.js"></script>
<?php $this->end() ?>

common-component-a.php

<?php $this->push('javascript') ?>
  <script src="common-component-a.js"></script>
<?php $this->end() ?>

The end result will be,

<script src="base-page-script-support-heavy-weight.js"></script>
<script src="common-component-a.js"></script>

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 this pull request may close these issues.

4 participants