Skip to content

[4.0] Child templates #30192

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.

Sign up for GitHub

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

Merged
wilsonge merged 21 commits into from
Aug 9, 2020
Merged

[4.0] Child templates #30192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9baf09c
Sweet child o’mine
dgrammatiko Jul 25, 2020
7eb0c83
Update base.sql
dgrammatiko Jul 25, 2020
f210d8b
CS
dgrammatiko Jul 25, 2020
9c8ec53
Remove uneeded code
dgrammatiko Jul 26, 2020
a210661
typo
dgrammatiko Jul 26, 2020
bf7fd52
Better naming
dgrammatiko Jul 26, 2020
cda7686
Fix the sql
dgrammatiko Jul 26, 2020
7d8e760
Consistency
dgrammatiko Jul 26, 2020
e7e9b0e
CS
dgrammatiko Jul 26, 2020
32302d5
Update installation/sql/postgresql/base.sql
dgrammatiko Jul 27, 2020
428ec11
Update installation/sql/postgresql/base.sql
dgrammatiko Jul 27, 2020
788d674
Update installation/sql/mysql/base.sql
dgrammatiko Jul 27, 2020
7e97cc8
Update administrator/components/com_admin/sql/updates/mysql/4.0.0-202…
dgrammatiko Jul 27, 2020
6cf49fc
Update installation/sql/mysql/base.sql
dgrammatiko Jul 27, 2020
55bdd41
Update InstallationApplication.php
dgrammatiko Jul 27, 2020
3d20ace
Update InstallationApplication.php
dgrammatiko Jul 27, 2020
0b4df24
Update CMSApplication.php
dgrammatiko Jul 27, 2020
3573ec4
oops
dgrammatiko Jul 27, 2020
a3603ca
Update libraries/src/Installer/Adapter/TemplateAdapter.php
dgrammatiko Jul 30, 2020
750fb26
Update libraries/src/Installer/Adapter/TemplateAdapter.php
dgrammatiko Jul 30, 2020
eec9d9a
Update libraries/src/MVC/View/HtmlView.php
dgrammatiko Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions administrator/components/com_admin/sql/updates/mysql/4.0.0-2020-06-25.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE `#__template_styles` ADD COLUMN `inheritable` tinyint(1) NOT NULL DEFAULT 0;
ALTER TABLE `#__template_styles` ADD COLUMN `parent` varchar(50) DEFAULT '';
2 changes: 2 additions & 0 deletions administrator/components/com_admin/sql/updates/postgresql/4.0.0-2020-06-25.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE "#__template_styles" ADD COLUMN "inheritable" smallint NOT NULL DEFAULT 0;
ALTER TABLE "#__template_styles" ADD COLUMN "parent" varchar(50) DEFAULT "";
Copy link
Member

@richard67 richard67 Aug 10, 2020

Choose a reason for hiding this comment

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

Next mistake DEFAULT "" should be DEFAULT ''

Copy link
Contributor

@alikon alikon Aug 10, 2020

Choose a reason for hiding this comment

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

please submit a fix i'll test it

Copy link
Member

@richard67 richard67 Aug 10, 2020

Choose a reason for hiding this comment

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

Done #30330 .

8 changes: 5 additions & 3 deletions installation/sql/mysql/base.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,8 @@ CREATE TABLE IF NOT EXISTS `#__template_styles` (
`client_id` tinyint(1) unsigned NOT NULL DEFAULT 0,
`home` char(7) NOT NULL DEFAULT '0',
`title` varchar(255) NOT NULL DEFAULT '',
`inheritable` tinyint(1) NOT NULL DEFAULT 0,
`parent` varchar(50) DEFAULT '',
`params` text NOT NULL,
PRIMARY KEY (`id`),
KEY `idx_template` (`template`),
Expand All @@ -793,9 +795,9 @@ CREATE TABLE IF NOT EXISTS `#__template_styles` (
-- Dumping data for table `#__template_styles`
--

INSERT INTO `#__template_styles` (`id`, `template`, `client_id`, `home`, `title`, `params`) VALUES
(10, 'atum', 1, '1', 'atum - Default', ''),
(11, 'cassiopeia', 0, '1', 'cassiopeia - Default', '{"logoFile":"","fluidContainer":"0","sidebarLeftWidth":"3","sidebarRightWidth":"3"}');
INSERT INTO `#__template_styles` (`id`, `template`, `client_id`, `home`, `title`, `inheritable`, `parent`, `params`) VALUES
(10, 'atum', 1, '1', 'atum - Default', 0, '', ''),
(11, 'cassiopeia', 0, '1', 'cassiopeia - Default', 0, '', '{"logoFile":"","fluidContainer":"0","sidebarLeftWidth":"3","sidebarRightWidth":"3"}');

-- --------------------------------------------------------

Expand Down
9 changes: 5 additions & 4 deletions installation/sql/postgresql/base.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,8 @@ CREATE TABLE IF NOT EXISTS "#__template_styles" (
"client_id" smallint DEFAULT 0 NOT NULL,
"home" varchar(7) DEFAULT '0' NOT NULL,
"title" varchar(255) DEFAULT '' NOT NULL,
`inheritable` smallint DEFAULT 0 NOT NULL,
`parent` varchar(50) DEFAULT '',
Comment on lines +801 to +802
Copy link
Member

@richard67 richard67 Aug 10, 2020

Choose a reason for hiding this comment

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

Strange that nobody saw this and nobody tested with PostgreSQL like it should always be done when a PR has database changes. This has broken installation on PostgreSQL. Will prepare a fix.

Copy link
Member

@richard67 richard67 Aug 10, 2020

Choose a reason for hiding this comment

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

Wong names quotes I mean.

Copy link
Member

@richard67 richard67 Aug 10, 2020

Choose a reason for hiding this comment

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

Fixed with #30327 .

"params" text NOT NULL,
PRIMARY KEY ("id")
);
Expand All @@ -808,10 +810,9 @@ CREATE INDEX "#__template_styles_idx_client_id_home" ON "#__template_styles" ("c
--
-- Dumping data for table `#__template_styles`
--

INSERT INTO "#__template_styles" ("id", "template", "client_id", "home", "title", "params") VALUES
(10, 'atum', 1, '1', 'atum - Default', ''),
(11, 'cassiopeia', 0, '1', 'cassiopeia - Default', '{"logoFile":"","fluidContainer":"0","sidebarLeftWidth":"3","sidebarRightWidth":"3"}');
INSERT INTO "#__template_styles" ("id", "template", "client_id", "home", "title", "inheritable", "parent", "params") VALUES
(10, 'atum', 1, '1', 'atum - Default', 0, '', ''),
(11, 'cassiopeia', 0, '1', 'cassiopeia - Default', 0, '', '{"logoFile":"","fluidContainer":"0","sidebarLeftWidth":"3","sidebarRightWidth":"3"}');

SELECT setval('#__template_styles_id_seq', 12, false);

Expand Down
11 changes: 7 additions & 4 deletions installation/src/Application/InstallationApplication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,8 @@ public function getTemplate($params = false)
$template = new \stdClass;
$template->template = 'template';
$template->params = new Registry;
$template->inheritable = 0;
$template->parent = null;

return $template;
}
Expand Down Expand Up @@ -544,10 +546,11 @@ public function render()
$file = $this->input->getCmd('tmpl', 'index');

$options = [
'template' => 'template',
'file' => $file . '.php',
'directory' => JPATH_THEMES,
'params' => '{}',
'template' => 'template',
'file' => $file . '.php',
'directory' => JPATH_THEMES,
'params' => '{}',
"templateInherits" => ''
];
}

Expand Down
27 changes: 20 additions & 7 deletions libraries/src/Application/AdministratorApplication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public function dispatch($component = null)
case 'html':
// Get the template
$template = $this->getTemplate(true);
$clientId = $this->getClientId();

// Store the template and its params to the config
$this->set('theme', $template->template);
Expand All @@ -121,7 +122,12 @@ public function dispatch($component = null)
$wr->addExtensionRegistryFile($component);
}

$wr->addTemplateRegistryFile($template->template, $this->getClientId());
if (!empty($template->parent))
{
$wr->addTemplateRegistryFile($template->parent, $clientId);
}

$wr->addTemplateRegistryFile($template->template, $clientId);

break;

Expand Down Expand Up @@ -223,8 +229,9 @@ public function getTemplate($params = false)

// Load the template name from the database
$db = Factory::getDbo();

$query = $db->getQuery(true)
->select($db->quoteName(['s.template', 's.params']))
->select($db->quoteName(['s.template', 's.params', 's.inheritable', 's.parent']))
->from($db->quoteName('#__template_styles', 's'))
->join(
'LEFT',
Expand Down Expand Up @@ -260,20 +267,26 @@ public function getTemplate($params = false)
$template->template = InputFilter::getInstance()->clean($template->template, 'cmd');
$template->params = new Registry($template->params);

if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
// Fallback template
if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php')
&& !file_exists(JPATH_THEMES . '/' . $template->parent . '/index.php'))
{
$this->enqueueMessage(Text::_('JERROR_ALERTNOTEMPLATE'), 'error');
$template->params = new Registry;
$template->template = 'atum';

// Check, the data were found and if template really exists
if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $template->template));
}
}

// Cache the result
$this->template = $template;

if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $template->template));
}
// Pass the parent template to the state
$this->set('themeInherits', $template->parent);

if ($params)
{
Expand Down
23 changes: 13 additions & 10 deletions libraries/src/Application/CMSApplication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,17 +561,19 @@ public static function getRouter($name = null, array $options = array())
*/
public function getTemplate($params = false)
{
$template = new \stdClass;

$template->template = 'system';
$template->params = new Registry;

if ($params)
{
$template = new \stdClass;

$template->template = 'system';
$template->params = new Registry;
$template->inheritable = 0;
$template->parent = '';

return $template;
}

return $template->template;
return 'system';
}

/**
Expand Down Expand Up @@ -939,10 +941,11 @@ public function redirect($url, $status = 303)
protected function render()
{
// Setup the document options.
$this->docOptions['template'] = $this->get('theme');
$this->docOptions['file'] = $this->get('themeFile', 'index.php');
$this->docOptions['params'] = $this->get('themeParams');
$this->docOptions['csp_nonce'] = $this->get('csp_nonce');
$this->docOptions['template'] = $this->get('theme');
$this->docOptions['file'] = $this->get('themeFile', 'index.php');
$this->docOptions['params'] = $this->get('themeParams');
$this->docOptions['csp_nonce'] = $this->get('csp_nonce');
$this->docOptions['templateInherits'] = $this->get('themeInherits');

if ($this->get('themes.base'))
{
Expand Down
53 changes: 50 additions & 3 deletions libraries/src/Application/SiteApplication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,11 @@ public function dispatch($component = null)
$wr->addExtensionRegistryFile($component);
}

if ($template->parent)
{
$wr->addTemplateRegistryFile($template->parent, $this->getClientId());
}

$wr->addTemplateRegistryFile($template->template, $this->getClientId());

break;
Expand Down Expand Up @@ -394,7 +399,17 @@ public function getTemplate($params = false)
{
if (\is_object($this->template))
{
if (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php'))
if ($this->template->parent)
{
if (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php'))
{
if (!file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php'))
{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $this->template->template));
}
}
}
elseif (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php'))
Comment on lines +402 to +412
Copy link
Member

@HLeithner HLeithner Jul 29, 2020

Choose a reason for hiding this comment

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

Suggested change
if ($this->template->parent)
{
if (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php'))
{
if (!file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php'))
{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $this->template->template));
}
}
}
elseif (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php'))
if (
!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')
&& (
empty($this->template->parent)
|| !file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php')
)
)

Copy link
Contributor Author

@dgrammatiko dgrammatiko Jul 30, 2020

Choose a reason for hiding this comment

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

@HLeithner the rest of the changes are fine but not this one. You can reduce the inner 2 conditionals to 1 but changing it to 1 (top) conditional is wrong in the sense that the ($this->template->parent) is extremely cheap to resolve but

!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')
				&& (
					empty($this->template->parent)
					|| !file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php')
				)

introduces 2 more file lookups for every execution for no reason.

Copy link
Member

@HLeithner HLeithner Jul 30, 2020

Choose a reason for hiding this comment

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

introduces 2 more file lookups for every execution for no reason.

actually not true, the second !file_exists will not be called if empty($this->template->parent) is true

but that was not the point you can leave it that way if you want it's only to reduce duplicated code. Will you merge the other suggestions?

Copy link
Contributor Author

@dgrammatiko dgrammatiko Jul 30, 2020

Choose a reason for hiding this comment

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

Done, although I have to state that I'm not so sure I want to move this one forward anymore.

Copy link
Member

@HLeithner HLeithner Jul 30, 2020

Choose a reason for hiding this comment

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

What's the reason? because I said it can not go into 4.1?

Copy link
Member

@HLeithner HLeithner Jul 31, 2020

Choose a reason for hiding this comment

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

I read them but the explanation but they don't make sense to me, a js that's template independent is something different then a image that's only used in the template. We already split everything about the joomla filesystem at least the templates are "self contained" but that's only my opinion.
Anyway that doesn't hold this feature back.

Copy link
Contributor

@SharkyKZ SharkyKZ Jul 31, 2020

Choose a reason for hiding this comment

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

The move to media folder seems unnecessary. Can't we just check for assets in child template directory and fallback to parent template directory?

If you want to move assets to media directory for other purposes, that could be done in a separate PR so it doesn't have to wait for this.

Copy link
Contributor Author

@dgrammatiko dgrammatiko Jul 31, 2020

Choose a reason for hiding this comment

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

You really don’t get it. It’s you only opportunity to fix this and bring consistency...

Copy link
Contributor

@Bakual Bakual Jul 31, 2020

Choose a reason for hiding this comment

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

You really don’t get it. It’s you only opportunity to fix this and bring consistency...

Do a PR where you only move the assets to media. I actually think that could still be done for 4.0 as it's not a new feature.
If it can be done independant, lets do it independant and don't mix things.

Copy link
Contributor Author

@dgrammatiko dgrammatiko Jul 31, 2020

Choose a reason for hiding this comment

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

Guys do you even realise the amount of work I put here to create all the 4 templates and the other html overrides for the tests?

Asking to break it in 2 is just asking me to repeat the whole work twice. Why? This is a new mode, it doesn't and shouldn't affect any of the current templates

{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $this->template->template));
}
Expand Down Expand Up @@ -453,8 +468,9 @@ public function getTemplate($params = false)
{
// Load styles
$db = Factory::getDbo();

$query = $db->getQuery(true)
->select($db->quoteName(['id', 'home', 'template', 's.params']))
->select($db->quoteName(['id', 'home', 'template', 's.params', 'inheritable', 'parent']))
->from($db->quoteName('#__template_styles', 's'))
->where(
[
Expand Down Expand Up @@ -529,7 +545,35 @@ public function getTemplate($params = false)
$template->template = InputFilter::getInstance()->clean($template->template, 'cmd');

// Fallback template
if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
if (!empty($template->parent))
Copy link
Member

@HLeithner HLeithner Jul 29, 2020

Choose a reason for hiding this comment

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

I think you can simplify this too like them above to reduce code duplication

{
if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
{
if (!file_exists(JPATH_THEMES . '/' . $template->parent . '/index.php'))
{
$this->enqueueMessage(Text::_('JERROR_ALERTNOTEMPLATE'), 'error');

// Try to find data for 'cassiopeia' template
$original_tmpl = $template->template;

foreach ($templates as $tmpl)
{
if ($tmpl->template === 'cassiopeia')
{
$template = $tmpl;
break;
}
}

// Check, the data were found and if template really exists
if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
{
throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $original_tmpl));
}
}
}
}
elseif (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php'))
{
$this->enqueueMessage(Text::_('JERROR_ALERTNOTEMPLATE'), 'error');

Expand Down Expand Up @@ -751,6 +795,9 @@ protected function render()
$this->set('themeFile', $file . '.php');
}

// Pass the parent template to the state
$this->set('themeInherits', $template->parent);

break;
}

Expand Down
19 changes: 15 additions & 4 deletions libraries/src/Document/HtmlDocument.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,13 +806,23 @@ protected function _fetchTemplate($params = array())
$filter = InputFilter::getInstance();
$template = $filter->clean($params['template'], 'cmd');
$file = $filter->clean($params['file'], 'cmd');
$inherits = $params['templateInherits'];
$baseDir = $directory . '/' . $template;

if (!file_exists($directory . '/' . $template . '/' . $file))
if (!empty($inherits)
&& !file_exists($directory . '/' . $template . '/' . $file)
&& file_exists($directory . '/' . $inherits . '/' . $file)
)
{
$baseDir = $directory . '/' . $inherits;
}

if (!file_exists($baseDir . '/' . $file))
{
$template = 'system';
}

if (!file_exists($directory . '/' . $template . '/' . $file))
if (!file_exists($baseDir . '/' . $file))
{
$file = 'index.php';
}
Expand All @@ -822,15 +832,16 @@ protected function _fetchTemplate($params = array())

// 1.5 or core then 1.6
$lang->load('tpl_' . $template, JPATH_BASE)
|| $lang->load('tpl_' . $inherits, $directory . '/' . $inherits)
|| $lang->load('tpl_' . $template, $directory . '/' . $template);

// Assign the variables
$this->template = $template;
$this->baseurl = Uri::base(true);
$this->params = $params['params'] ?? new Registry;
$this->template = $template;

// Load
$this->_template = $this->_loadTemplate($directory . '/' . $template, $file);
$this->_template = $this->_loadTemplate($baseDir, $file);

return $this;
}
Expand Down
Loading