Admin app - Use the autoloader #10881

mbabker · 2016-06-20T21:19:29Z

Summary of Changes

Load files in the admin app by including the required classes to the autoloader.

Testing Instructions

The admin app should still function correctly.

dgrammatiko · 2016-06-20T23:10:29Z

administrator/components/com_banners/models/forms/filter_banners.xml

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <form>
-	<fields name="filter">
+	<fields name="filter" addfieldpath="/administrator/components/com_banners/models/fields">


@mbabker isn't this supposed to autoload by https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/form.php#L203-L206 ?

If you only use these forms going through JModelForm objects then sure. If
they load any other way the dependency wouldn't be loaded. I equate it to
relying on something else to load jQuery when you depend on it.

On Monday, June 20, 2016, Dimitri Grammatikogianni notifications@github.com
wrote:

In administrator/components/com_banners/models/forms/filter_banners.xml
#10881 (comment):

@@ -1,6 +1,6 @@
- -

@mbabker https://github.com/mbabker isn't this supposed to autoload by
https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/form.php#L203-L206
?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/10881/files/5f19de3d1f3851054f65ed03611262be65340315#r67783423,
or mute the thread
https://github.com/notifications/unsubscribe/AAWfoeL9oXQo7eQDLYLivxj4CStNBLhUks5qNx3ngaJpZM4I6I8J
.

pritalpatel · 2016-07-02T14:01:16Z

I have tested this item ✅ successfully on b7afe6c

I have tested it using automatic test suits which I have created to test content and user feature. All were successful after applying this patch. Created video for this tests here https://youtu.be/D4TKK4VUIHg

(Regarding testing instructions first I was confused by word "admin app" in testing info. But later I understand that it's mean to Joomla! Administrator tests)

Thanks.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10881.}

shubhamnba2009 · 2016-07-02T14:50:50Z

administrator/components/com_banners/views/tracks/view.html.php

@@ -66,8 +66,6 @@ public function display($tpl = null)

 		$this->addToolbar();

-		require_once JPATH_COMPONENT . '/models/fields/bannerclient.php';
-


Hi @mbabker
Why JLoader::register() is not used after removing this code?

I could not find a direct use of the class that this was loading in the code. Meaning either it's being loaded as part of a form (where the lookup path should be included in the XML or added with a JFormHelper::addFieldPath() call) or it was being loaded needlessly.

alikon · 2016-07-03T05:53:52Z

I have tested this item ✅ successfully on b7afe6c

with this patch applyed for a couple of days the admin side still works as expected

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10881.}

brianteeman · 2016-07-03T07:10:40Z

rtc

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10881.}

andrepereiradasilva · 2016-07-17T13:07:59Z

administrator/components/com_menus/models/item.php

@@ -12,7 +12,7 @@
 use Joomla\Registry\Registry;

 jimport('joomla.filesystem.path');
-require_once JPATH_COMPONENT . '/helpers/menus.php';
+JLoader::register('MenusHelper', JPATH_COMPONENT . '/helpers/menus.php');


@mbabker please see #11164

In general that's why the JPATH_COMPONENT path constants should only serve one purpose in Joomla until 4.0 is released; raise deprecation notices. Those are in part why it is so freakin' difficult to work between different components, because that path is not constant by any means.

joomla-cms-bot · 2016-07-17T15:42:41Z

This PR has received new commits.

CC: @alikon, @pritalpatel

infograf768 · 2016-07-17T19:13:40Z

@mbabker
looks like a typo for com_menus
com_media instead of com_menus for the loader

mbabker · 2016-07-17T19:16:23Z

Fixed. Copy/Paste errors suck.

andrepereiradasilva · 2016-07-17T19:38:23Z

administrator/components/com_modules/controller.php

@@ -57,7 +57,7 @@ public function display($cachable = false, $urlparams = false)
 			return $view->display();
 		}

-		JLoader::register('ModulesHelper', JPATH_COMPONENT . '/helpers/modules.php');
+		JLoader::register('ModulesHelper', JPATH_ADMINISTRATOR . '/components/com_messages/helpers/modules.php');


com_modules, not com_messages

ggppdk · 2016-07-18T05:52:46Z

administrator/components/com_installer/models/fields/extensionstatus.php

@@ -11,7 +11,7 @@

 JFormHelper::loadFieldClass('list');

-require_once __DIR__ . '/../../helpers/installer.php';
+JLoader::register('InstallerHelper', JPATH_ADMINISTRATOR . '/components/com_installer/helpers/installer.php');


This is right, but it is now using JPATH_ADMINISTRATOR+component_folder, it could have kept the usage of __ DIR __

JLoader::register('InstallerHelper', dirname(__DIR__, 2) . '/helpers/installer.php');

I say this because in other cases you keep the usage of __ DIR __, but in some you do not

I personally prefer absolute paths over relative when practical. I
dislike trying to read paths in include statements with directory traversal
like that because I think it makes it harder to follow.

The second param on dirname() is only present as of PHP 7.0.

On Monday, July 18, 2016, Georgios Papadakis notifications@github.com
wrote:

In
administrator/components/com_installer/models/fields/extensionstatus.php
#10881 (comment):

@@ -11,7 +11,7 @@

JFormHelper::loadFieldClass('list');

-require_once DIR . '/../../helpers/installer.php';
+JLoader::register('InstallerHelper', JPATH_ADMINISTRATOR . '/components/com_installer/helpers/installer.php');

This is right, but it is now absolute path, it could have remain a
"relative" using the DIR:

JLoader::register('InstallerHelper', dirname(DIR, 2) . '/helpers/installer.php');

I say this because in other cases you keep the usage of DIR, but in
some you do not

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/10881/files/ec8218c23101f868671104e3c8505f357337677d#r71102143,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoRDLWy8U5Vzyv4f6C78Q2lDDpyBoks5qWxSvgaJpZM4I6I8J
.

infograf768 · 2016-07-18T08:38:27Z

@mbabker
I am also curious: why use DIR in some cases and full path in others?
Shall we not use all over the fullpath?

mbabker · 2016-07-18T11:57:33Z

The DIR constant is an absolute path, specifically to the directory the
file is located in. IMO it's more readable to use it as able (mostly when
the path being loaded is the same level or under the file referencing it).

On Monday, July 18, 2016, infograf768 notifications@github.com wrote:

@mbabker https://github.com/mbabker
I am also curious: why use DIR in some cases and full path in others?
Shall we not use all over the fullpath?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10881 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfobpl7lMnlKTAhJnWvAnw8eU3lqbCks5qWzuFgaJpZM4I6I8J
.

ggppdk · 2016-07-18T14:26:11Z

@mbabker

The DIR constant is an absolute path, specifically to the directory the file is located in.

yes about "absolute path" , it is

but the benefit of it is that you don't need to care much about what is exactly this absolute path is,
it helps avoiding the hardcoding of component name
e.g.
/components/com_installer/

effectively behaving as relative paths are used

the argument , if same folder or parent folder, then we use it __ DIR __, but if parent - parent folder we don't use , sounds strange to me

mbabker · 2016-07-18T14:30:48Z

It's a personal preference and AFAIK there is no style rule regarding how paths should be referenced. I'd suggest either absolute paths using __DIR__ or one of the JPATH_ constants referencing an application root, but I personally dislike require __DIR__ . '/../../folder/file.php' structures and avoid using them. As for when I use JPATH_ or __DIR__ it really boils down to where the file in question lies in the filesystem more than anything; I don't see the need to write out the full path if I'm including a file in either the same directory or a subdirectory from the file I'm in.

If we're going to nitpick over this stuff, I'll just close my autoloader PRs. I'm also not a fan of PRs getting held up because people want to try and apply arbitrary standards to them.

infograf768 · 2016-07-18T14:31:53Z

I have tested this item ✅ successfully on ec8218c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10881.}

mbabker · 2016-07-18T14:32:58Z

Also, before I end ranting, a benefit to using absolute paths versus relative is you don't have to change the include reference if you move the file. If I move the file calling require __DIR__ . '/../../folder/file.php' to another directory, I have to update the reference (OK, same goes for anything using __DIR__ honestly; using JPATH_ avoids that completely but IMO it's unnecessary overkill sometimes).

infograf768 · 2016-07-18T14:33:50Z

anyhow, this works fine here and solves the issue we had with the gsoc project.
@andrepereiradasilva can you test again so that we get this in staging?

andrepereiradasilva · 2016-07-18T15:10:54Z

I have tested this item ✅ successfully on ec8218c

on code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10881.}

ggppdk · 2016-07-18T15:12:48Z

Also, before I end ranting, a benefit to using absolute paths versus relative is you don't have to change the include reference if you move the file. If I move the file calling require DIR . '/../../folder/file.php' to another directory, I have to update the reference (OK, same goes for anything using DIR honestly; using JPATH_ avoids that completely but IMO it's unnecessary overkill sometimes).

@mbabker
Agreed that is why i prefer to use JPATH too

easier to copy or move the code
easier to read (for me)

I'm also not a fan of PRs getting held up because people want to try and apply arbitrary standards to them.

and i hope that you don't imply that i am preventing a good PR, i was just asking, i had said the paths are good.

mbabker · 2016-07-18T15:13:50Z

Not trying to imply anything at all, sorry if it comes across that way. Just seen a lot more than I'd like lately folks requesting changes to PRs simply because they're already editing the lines they'd like to see changed.

Admin app - Use the autoloader

5f19de3

joomla-cms-bot added the PR-staging label Jun 20, 2016

dgrammatiko reviewed Jun 20, 2016
View reviewed changes

Michael Babker added 2 commits June 21, 2016 12:23

Add includes to this as well

32bccc4

Added more includes to search

b7afe6c

shubhamnba2009 reviewed Jul 2, 2016
View reviewed changes

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 3, 2016

andrepereiradasilva reviewed Jul 17, 2016
View reviewed changes

Don't use the component path unconstants

f61b29e

andrepereiradasilva mentioned this pull request Jul 17, 2016

Correct model helpers include paths (com_menus item, com_user debuguser and debuggroup) #11164

Closed

It helps to reference the right component

f3018d9

andrepereiradasilva reviewed Jul 17, 2016
View reviewed changes

Grr...

ec8218c

ggppdk reviewed Jul 18, 2016
View reviewed changes

wilsonge merged commit a0f1f5a into joomla:staging Jul 18, 2016

wilsonge added this to the Joomla 3.6.1 milestone Jul 18, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 18, 2016

mbabker deleted the Use-The-Autoloader branch July 18, 2016 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin app - Use the autoloader #10881

Admin app - Use the autoloader #10881

mbabker commented Jun 20, 2016 •

edited by brianteeman

Loading

dgrammatiko Jun 20, 2016

mbabker Jun 21, 2016

pritalpatel commented Jul 2, 2016

shubhamnba2009 Jul 2, 2016

mbabker Jul 2, 2016

alikon commented Jul 3, 2016

brianteeman commented Jul 3, 2016

andrepereiradasilva Jul 17, 2016

mbabker Jul 17, 2016

joomla-cms-bot commented Jul 17, 2016

infograf768 commented Jul 17, 2016

mbabker commented Jul 17, 2016

andrepereiradasilva Jul 17, 2016

mbabker Jul 17, 2016

ggppdk Jul 18, 2016 •

edited

Loading

mbabker Jul 18, 2016

infograf768 commented Jul 18, 2016

mbabker commented Jul 18, 2016

ggppdk commented Jul 18, 2016

mbabker commented Jul 18, 2016

infograf768 commented Jul 18, 2016

mbabker commented Jul 18, 2016

infograf768 commented Jul 18, 2016

andrepereiradasilva commented Jul 18, 2016

ggppdk commented Jul 18, 2016 •

edited

Loading

mbabker commented Jul 18, 2016

		@@ -66,8 +66,6 @@ public function display($tpl = null)

		$this->addToolbar();

		require_once JPATH_COMPONENT . '/models/fields/bannerclient.php';

Admin app - Use the autoloader #10881

Admin app - Use the autoloader #10881

Conversation

mbabker commented Jun 20, 2016 • edited by brianteeman Loading

Summary of Changes

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritalpatel commented Jul 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikon commented Jul 3, 2016

brianteeman commented Jul 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joomla-cms-bot commented Jul 17, 2016

infograf768 commented Jul 17, 2016

mbabker commented Jul 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggppdk Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infograf768 commented Jul 18, 2016

mbabker commented Jul 18, 2016

ggppdk commented Jul 18, 2016

mbabker commented Jul 18, 2016

infograf768 commented Jul 18, 2016

mbabker commented Jul 18, 2016

infograf768 commented Jul 18, 2016

andrepereiradasilva commented Jul 18, 2016

ggppdk commented Jul 18, 2016 • edited Loading

mbabker commented Jul 18, 2016

mbabker commented Jun 20, 2016 •

edited by brianteeman

Loading

ggppdk Jul 18, 2016 •

edited

Loading

ggppdk commented Jul 18, 2016 •

edited

Loading