Load correct core files of override files #2

anuragteapot · 2018-05-21T14:10:56Z

Pull request for the issue #1

Load correct core files of override files.

laoneo · 2018-05-22T07:57:08Z

administrator/components/com_templates/Controller/TemplateController.php

+	 *
+	 * @since   4.0.0
+	 */
+	public function loadCoreFiles()


Is this function used somewhere?

No, I made this for function testing.

Should I remove this ?

I would so then.

laoneo · 2018-05-22T07:58:11Z

administrator/components/com_templates/Model/TemplateModel.php

+	public function loadCoreFile($file, $client_id)
+	{
+		$app           = Factory::getApplication();
+		$filePath      = base64_decode($file);


This should be done in the cistructor, in the model you can assume that the file is a proper string.

laoneo · 2018-05-22T08:11:15Z

administrator/components/com_templates/Model/TemplateModel.php

+	{
+		$app           = Factory::getApplication();
+		$filePath      = base64_decode($file);
+		$explodeArray  = explode(DIRECTORY_SEPARATOR, $filePath);


I fear that this can give us OS issues sooner or later.

We can use $path = \JPath::clean($path); before this, then i think it should be fine ?

Yes, JPath can be your friend here.

laoneo · 2018-05-22T08:11:46Z

administrator/components/com_templates/Model/TemplateModel.php

+			return false;
+		}
+
+		$fileName = end($explodeArray);


You can use basename() here.

Is basename safe ?

It is a core function, so hopefully it is.

laoneo · 2018-05-22T08:12:40Z

administrator/components/com_templates/Model/TemplateModel.php

+			$extension = File::getExt($name);
+
+			// Remove ( Date ) from file
+			$explodeArray         = explode('-', $name);


How do we get a date into the file name?

Try to create one override two times, then it add unique name by adding current Date to it .

Didn't know that!

laoneo · 2018-05-22T08:13:44Z

administrator/components/com_templates/Model/TemplateModel.php

@@ -76,10 +82,10 @@ public function getFiles()
 		if ($template = $this->getTemplate())
 		{
 			jimport('joomla.filesystem.folder');
-			$app    = \JFactory::getApplication();


Namespacing should be done against the main repo. Like that we can keep the pr's here small and will have less possible merge conflicts.

should I revert this?

If ok for you, then I would revert all the namespacing. When we create the pr then for the main repo it is easier to review then.

anuragteapot · 2018-05-22T13:54:10Z

@laoneo Done!

laoneo · 2018-05-22T14:14:38Z

administrator/language/en-GB/en-GB.com_templates.ini

@@ -73,6 +73,8 @@ COM_TEMPLATES_ERROR_SOURCE_ID_FILENAME_MISMATCH="Stored ID does not match the su
 COM_TEMPLATES_ERROR_STYLE_NOT_FOUND="Style not found."
 COM_TEMPLATES_ERROR_STYLE_REQUIRES_TITLE="The style requires a title."
 COM_TEMPLATES_ERROR_TEMPLATE_FOLDER_NOT_FOUND="Template folder not found."
+COM_TEMPLATES_ERROR_CORE_FILE_NOT_FOUND="Core file not found."


Strings need to be alpha ordered.

laoneo · 2018-05-22T14:15:36Z

administrator/components/com_templates/Model/TemplateModel.php

+	 *
+	 * @return mixed  $path The full path and file name for the target file, or boolean false if the file is not found in any of the paths.
+	 *
+	 * @since  4.0.0


Since tags should be __DEPLOY_VERSION__ we actually do not know in which version this feature will land at the end.

astridx · 2018-05-22T20:01:51Z

administrator/components/com_templates/Model/TemplateModel.php

@@ -10,10 +10,14 @@

 defined('_JEXEC') or die;

-use Joomla\CMS\Application\ApplicationHelper;
-use Joomla\CMS\Component\ComponentHelper;


Is this the same as @laoneo wrote regards to \JFactory in the Glip Channel? Should not we do the re-sorting and only make the necessary changes so that we have fewer conflicts?

If Anurag is using PHPStorm, then the import sorting is automatically done by the IDE. In the new code it is absolutely fine to use the namespaced classes. It is fine for me.

astridx · 2018-05-22T21:25:13Z

administrator/components/com_templates/Model/TemplateModel.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	public function loadCoreFile($file, $client_id)


Do we have to sort the methods alphabetically?
Should we call the method getCoreFile instead of loadCoreFile?

Alphanumeric function sorting is not required in Joomla.

astridx · 2018-05-22T22:10:55Z

administrator/components/com_templates/Model/TemplateModel.php

+		$type     = $explodeArray['2'];
+		$client   = ApplicationHelper::getClientInfo($client_id);
+
+		$componentPath = Path::clean($client->path . '/components/');


Can we set the $componentPath only if type is component and the $layoutPath only if type is layout ...?

Yes, we can. Should i do ?

$componentPath is used two section in Jlayout and Component
So, I think it's fine.

astridx · 2018-05-22T22:11:15Z

administrator/components/com_templates/Model/TemplateModel.php

+		$layoutPath    = Path::clean(JPATH_ROOT . '/layouts/');
+
+		// For modules
+		if (stristr($type, 'mod_') != false)


Should we use !== instead of != ?

Yes, we have to. plg_mymod_awesome would also go through here. Good catch.

@laoneo how we get plg_mymod_awesome ?

astridx · 2018-05-22T22:46:58Z

administrator/components/com_templates/Model/TemplateModel.php

+	 */
+	private function getSafeName($name)
+	{
+		if (preg_match('/[0-9]/', $name))


I like the fact that you make the diff view possible for the files with attached date.
Maybe we can do that even better.
At the moment, there must be a number in the name. You probably wanted to check if a date was attached. This test is in my opinion not necessary.
Already for the file /html/mod_search/default-red0-red.php the diff view modules/ mod_search/tmpl/default.php would be found.
Maybe we just have to look for a minus. Then a diff view would also be displayed for the files /html/mod_search/default-red.php and /html/mod_search/default-green.php - in case that the user renamed the file.

@astridx Yeah, you are right my first approach is based on minus signs. But, it fails here because action-button.php is the core file.

I think action button is the only file with a minus. All others are separated by an underscore. Is there a rule that a file should not contain a minus sign unless it is overwritten twice and the date is appended? Or can we ignore this one file (action-button)? So we could also offer the diff view for the doubly overwritten files. Otherwise, I do not see any rule that we can safely identify duplicate files. Then we should limit ourselves to the really equal written files.

Already for the file /html/mod_search/default-red0-red.php the diff view modules/ mod_search/tmpl/default.php would be found.

Is we have to treat default-red0-red.php as core file default.php ?

One more example.
@astridx In my function. I think we have to add one more check of date format . Then, it should fine :)

May be, this can help here http://php.net/manual/en/function.checkdate.php#113205

anuragteapot · 2018-05-23T09:24:48Z

@astridx @laoneo please check my last commit I add more validation in the name.
Before, loads default-red0-red.php as core file default.php

Now, it's working fine.

laoneo · 2018-05-23T09:39:08Z

For me this is fine for now. I'm sure we need more tweaks here in the future, but to not hold us back I would merge it.

update com_csp

* Load correct core files of override files (#2) Start implements loadcorefile() in administrator/components/com_templates/Model/TemplateModel.php * CS (#3) Coding Standards * codingstandards * codingstandards (#4) * Test (#6) Phase 2 (2 part) Mechanism to find correct core file and implementation. * Remove Notice: Only available for html-folder * Remove Warning if core file not found (#11) Thanks. So one part of the issue #12 is done. * Implement the diff view in template manager Implement the diff view in template manager * coding standard (#17) * fix diff (#18) Fix bug in path in case of administrator template override. Fix bug in path in case of administrator template override. * Notification after update and TEST (#16) Find changed files of overridden files and show message. * coding standard (#21) * correction * correction (#26) * Correcthtmlpath (#27) * correction * change oldhtml to newhtml * List of updated override files. (#30) * addcss (#34) * Final Product (#39) Core and Diff view Updated override history list. Quick icon notification plugin. Override control plugin. * save 3 lines :) * New feature show status. (#47) show status in com_template view templates * link * corrected namespace * Button to Switch (#35) * wip add Switcher * wip style switcher * wip style switch make inline and change on off text * wip start with js * wip js * wip delete buttons and make js more robust * wip save to storage * wip delete old code * wip * wip lint * wip css * set default value for switcher * wip make switcher blue * wip * wip * build * correct names * create new functions * fist test code * use onchange * undo installer.min.js * add forgotten new line at the end of css file * correct align * correct compare.es6 - only deleted the toggle part * correct compare.js - only deleted the toggle part * wip * reduce timeout * wrap in funcitons * wip * add use strict to both js-files(compare and toggle) * add the timeout value of 500 again, because 200 are not enought in my case * use css class 'active' for toggle views * add strict * time out for editor * wip * improvments use newActive and switch * correction * width of switcher-spans * correct align * do not use global * wip * removed timeouts * JTEXT to TEXT * forgotton last line * deleted duplicated comments * css fix align * use unnamed functions in es6 * Sql files for fix database (#50) * sql files for database fix * delete space * Suggestion for displaying Dates in view updates files (#52) Correct Dates and do not use date of file any more * Store Date as UTC and show it in server time zone (#57) * modified and created date are created and stored in UTC * convert dates for displaying in model * spar a loop * normalize timezone in view * use language constants for dateformat * JToolbarHelper to ToolbarHelper * CS * namespace * plural * name * clean * text * fx * sin * files * s * Suggestion for language strings (#60) * language strings * correct typo * delete media folder plg_quickicon * add folder plg_quickicon to build/media_src * delete files in media folder * Move media folder - System (#66) * multi * cs * delete files in media folder for joomla toolbar (#67) * Fix button switchers style. (#70) * button * CS * changed uitab.addTab for updated files * Bring back core.js changes. (#69) * core.js * const * fix * form * core * hound * CS * scopr * grid * alpha * cs * lang * only override file * lang * override lang installer * Cs * sub * Update list of core extensions (#71) * Language changes (#76) * update * Update en-GB.com_templates.ini * override JLIB_HTML_PUBLISH_ITEM this is the hover text on the publish icon in the list of files * Change icon (#74) change the icon to use an outline for more consistency * lang * not core (#75) * not core * Update en-GB.plg_installer_override.ini * namespace * cs * Updated files (#82) * Update default_updated_files.php * Update en-GB.com_templates.ini * Update en-GB.com_templates.ini (#81) * Update en-GB.plg_quickicon_overridecheck.ini (#80) * Update en-GB.plg_quickicon_overridecheck.ini (#79) * remove space (#78) * Update en-GB.plg_quickicon_overridecheck.ini * Update en-GB.plg_quickicon_overridecheck.sys.ini * remove hardcoded id * null get function * state * clean * More changes "core" to "original" (#85) * cs * update * plural

Anu1601CS added 3 commits May 21, 2018 19:15

corefile

bf2b69f

Add language string

83845ae

CSC

addc3f5

laoneo reviewed May 22, 2018

View reviewed changes

some fixes

fede0f2

laoneo reviewed May 22, 2018

View reviewed changes

laoneo added the feature New feature label May 22, 2018

Add __DEPLOY_VERSION__

1815544

anuragteapot requested a review from astridx May 22, 2018 14:54

anuragteapot self-assigned this May 22, 2018

astridx reviewed May 22, 2018

View reviewed changes

some improvement

cea3607

laoneo approved these changes May 23, 2018

View reviewed changes

astridx merged commit 3610beb into joomla-projects:master May 23, 2018

anuragteapot deleted the corefile branch May 25, 2018 01:03

zero-24 pushed a commit that referenced this pull request Jun 20, 2018

Merge pull request #2 from zero-24/cspj4

e71cfa6

update com_csp

Load correct core files of override files #2

Load correct core files of override files #2

Conversation

anuragteapot commented May 21, 2018

Pull request for the issue #1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuragteapot commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuragteapot May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuragteapot May 23, 2018 • edited Loading

Choose a reason for hiding this comment

anuragteapot commented May 23, 2018 • edited Loading

laoneo commented May 23, 2018

anuragteapot May 23, 2018 •

edited

Loading

anuragteapot May 23, 2018 •

edited

Loading

anuragteapot commented May 23, 2018 •

edited

Loading