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

[4.0] Move template files to tmpl folder #16218

Merged
merged 8 commits into from
May 29, 2017

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented May 23, 2017

Summary of Changes

With the move to namespaces, we didn't touch the location of the component template files, eg. default.php. They are still in a folder like /components/com_content/views/article/tmpl. This location was valid when we had the view.html.php in the folder above. Now the class is in a different folder. So I think the template files should be moved to a location where it makes more sense and is inline with the rest of the core like modules or plugins.

This pr moves the template files to a tmpl folder within the component root folder below the view name. So /components/com_content/views/article/tmpl/default.php becomes /components/com_content/tmpl/article/default.php.

The menu manager was adopted to support that new folder scheme as well as the view.

This pr was inspired by #15221 and is fully BC as all the other views should render as before. If this pr get accepted, we need to move all the template files of all the namespaced components.

Testing Instructions

  • Open an article in the back end.
  • Create a new menu item with a "Single Article"
  • Create a template override in the template manager for the article view and another one.

Expected result

All should work as before. Also the template overrides should work.

Actual result

Nothing is broken all should work.

Documentation Changes Required

This new location needs to be documented.

@mbabker mbabker added this to Testing/Review in [4.0] MVC Layer May 23, 2017
@brianteeman
Copy link
Contributor

wont the code in com_templates that creates overrides need changing as well?

@laoneo
Copy link
Member Author

laoneo commented May 23, 2017

Nope, overrides are the same.

@mbabker
Copy link
Contributor

mbabker commented May 23, 2017

It would need to know where to look to find the overrides though.

@dgrammatiko
Copy link
Contributor

@mbabker is there any plan to refactor the override creator part?

@laoneo
Copy link
Member Author

laoneo commented May 23, 2017

I'v just tested it and overrides do work as before.

@dgrammatiko
Copy link
Contributor

@laoneo creating an override?

@laoneo
Copy link
Member Author

laoneo commented May 23, 2017

Yes

@brianteeman
Copy link
Contributor

It would need to know where to look to find the overrides though.

how does it know where to look if the files have moved

@dgrammatiko
Copy link
Contributor

@laoneo
Copy link
Member Author

laoneo commented May 23, 2017

Why do you guys don't believe me, it works 😩

@brianteeman
Copy link
Contributor

@dgt41 that code is for creating the files and folders - i am talking about the code that finds the views that can be overriden

@brianteeman
Copy link
Contributor

thanks @mbabker

@brianteeman
Copy link
Contributor

and reading that code I cant see how it can find a view in the new location - but thats probably my lack of skill at reading code if @laoneo says it does. Anyway this is what I was referring to in my first comment

@laoneo
Copy link
Member Author

laoneo commented May 24, 2017

This change here https://github.com/joomla/joomla-cms/pull/16218/files#diff-c0ed342062e86e3c3f81da894f9d3e8fR137 adds the tmpl path to the file lookup array.

@joomdonation
Copy link
Contributor

@laoneo I think Brian is talking about the code which is used to create override in Template Manager. See the block of code which Michael mentioned https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_templates/models/template.php#L569-L597

Since the layout is now moved to new location, we need to adjust that block of code to allow template manager to find layout from new location

@laoneo
Copy link
Member Author

laoneo commented May 24, 2017

I was thinking I do miss something. Will have a look on that as well.

@brianteeman
Copy link
Contributor

Yes that's exactly what I meant

@laoneo
Copy link
Member Author

laoneo commented May 24, 2017

Changed the override editor to support the new scheme as well. Updated the test instructions.

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on b9b0e6d

creating overrides is not building the path correctly if the site is installed in a subdirectory as it is including the subdirectory that the site is installed in


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16218.

@brianteeman
Copy link
Contributor

this screenshot shows an override created before applying this pr at the bottom and after applying this pr at the top. you will see that my site is installed in a subdirectory called cms4

screenshotr10-46-56

@laoneo
Copy link
Member Author

laoneo commented May 27, 2017

I just tested it and it again and I'm working in a subfolder as well, it worked as it should. For a test can you install it from here https://github.com/Digital-Peak/joomla-cms/archive/j4/move-views-tmpl.zip, just to be sure the full patch get applied.

@brianteeman
Copy link
Contributor

Retested as requested with your zip - no change

screenshotr19-32-19

@laoneo
Copy link
Member Author

laoneo commented May 27, 2017

Perhaps somebody else can test it as I don't know what to do more. Guess we will have another look on the code sprint as well. Thanks so far for testing.

@joomdonation
Copy link
Contributor

I don't see the problem like Brian mentioned, but there are still issues with creating overrider:

  1. For com_users (old layout structure), the override is created at html\view\com_users\views\login . It should be html\com_users\login only

  2. For com_content (new layout structure), the override is created at html\view\com_content\tmpl\article, it should be html\com_content\article only

@joomdonation
Copy link
Contributor

Forgot to mention I am using Windows. And I installed your branch for testing. So something is still not correct yet

@brianteeman
Copy link
Contributor

@joomdonation I am on windows too

}

$result['components'][$component][] = $this->getOverridesFolder($view, $folder . '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this line to

$result['components'][$component][] = $this->getOverridesFolder($view, \JPath::clean($folder . '/'));

fixed the problem which I reported (on windows)

@joomdonation
Copy link
Contributor

Also, maybe the code in foreach block of getOverridesList method could be changed to:

foreach ($components as $component)
{
	$requireTmplFolder = true;

	if (file_exists($componentPath . '/' . $component . '/tmpl/'))
	{
		$viewPath = \JPath::clean($componentPath . '/' . $component . '/tmpl/');
		$requireTmplFolder = false;
	}
	elseif (file_exists($componentPath . '/' . $component . '/views/'))
	{
		$viewPath = \JPath::clean($componentPath . '/' . $component . '/views/');
	}
	elseif (file_exists($componentPath . '/' . $component . '/view/'))
	{
		$viewPath = \JPath::clean($componentPath . '/' . $component . '/view/');
	}
	else
	{
		$viewPath = '';
	}
	
	if ($viewPath)
	{
		$views = \JFolder::folders($viewPath);

		foreach ($views as $view)
		{
			// Only show the view has layout inside it
			if (!$requireTmplFolder || file_exists($viewPath . $view . '/tmpl'))
			{
				$result['components'][$component][] = $this->getOverridesFolder($view, $viewPath);
			}
		}
	}
}

I think it is less change, easier to understand (maybe just me) and save \JFolder::folders call on unnecessary folders.

@brianteeman
Copy link
Contributor

the override creation works for me now


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16218.

@laoneo
Copy link
Member Author

laoneo commented May 29, 2017

Good to hear

@wilsonge wilsonge merged commit f34f7c7 into joomla:4.0-dev May 29, 2017
@wilsonge wilsonge deleted the j4/move-views-tmpl branch May 29, 2017 09:29
@wilsonge wilsonge added this to the Joomla 4.0 milestone May 29, 2017
@wilsonge
Copy link
Contributor

Seems good to me. Let's get the rest of the core component converted :)

@mbabker mbabker moved this from Testing/Review to Completed in [4.0] MVC Layer May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants