Introduce predefined path in plugins, modules, templates for custom fields in the manifest file #4294

dgrammatiko · 2014-09-17T20:50:30Z

Simple addition to allow components, plugins, modules and templates to use custom fileds in a sub-directory named `fields`.

This is an alternative way to use custom fields in these areas besides the classic hardcoding addfieldpath=“wherever". Should make things a bit easier for devs without introducing complicated and slow procedures (only one directory check is actually used).

Pros

Predefined workflow
Easier implementation
No need for hardcoded paths in xml
Unified method that works along the existing hand coded addfieldpath=“wherever"

Against

Adds one more directory with predefined name
Adds some more calls for every template, module, plugin in the pile of execution code.

There shouldn't be any B/C breaks here.

How to test

For plugins:
apply this patch
apply also patch #4240
open /plugins/editors/tinymce/tinymce.xml and remove:
addfieldpath="/plugins/editors/tinymce/fields”
from the line:
<fieldset name="basic" addfieldpath="/plugins/editors/tinymce/fields”>
Go to admin and observe that the fields site skins and administrator skins are still functional.

For templates check the instructions at #4295

For modules repeat the instructions of plugins but for any module you choose. For a custom field code just use the one from the tinymce and it should work (you only need to see it rendering the functionality is not essential here). So for mod_breadcrumbs:
open mod_breadcrumbs.xml and paste
<field name="skin_admin" type="skins" description="PLG_TINY_FIELD_SKIN_ADMIN_DESC" label="PLG_TINY_FIELD_SKIN_ADMIN_LABEL" > </field>
under the fieldset=“basic”
copy the folder fields from the plugin tinimce [the plugin test abobe]
check in the admin module breadcrumps for the new option

For components:
This is already the standard for components. Custom fields expected to be found at com_name/models/fields

Also try that 3PD plugins, modules and templates don't misbehave!

Simple addition to allow plugins to use custom fileds in a directory named ```fields```. There shouldn't be any B/C breaks here.

Bakual · 2014-09-18T06:14:27Z

I'm not sure what the purpose is. It's already possible to use custom fields by providing the addfieldpath to the XML.
If I understand that correct, you just want to automate this so you don't have to define the path yourself?

dgrammatiko · 2014-09-18T08:57:58Z

@Bakual three things are done with this simple line of code:

Code doesn't need to be hard coded in xml's files
We get a unified way to do things
It makes things easier for dev's

Bakual · 2014-09-18T10:20:52Z

Code doesn't need to be hard coded in xml's files

Hmm, so instead of "hardcoding" the path it in the XML, we hardcode it in the model.

We get a unified way to do things

Actually, you introduce a new way to do things. Now one could either use the predefined path or specify an own in the XML. But only in plugins and templates. Components and modules would need additional PRs to be in line with this change, right?

It makes things easier for dev's

A tiny bit, yes. However I have to say it's very simple to add this attribute to the XML. This option needs to stay anyway, because extensions may have their fields in a library or other places.

I'm not sure if I like it or not. It's only a tiny gain imho at the cost of adding a tiny bit of additional complexity to the system. Let's see what others think.

Can you maybe adjust the title and description of the PR? Currently it's misleading as it has nothing to do with allowing custom fields for plugins (or templates). That is already possible without any issues by providing the addfieldpath attribute in the XML.

dgrammatiko · 2014-09-18T11:28:22Z

@Bakual

Hmm, so instead of "hardcoding" the path it in the XML, we hardcode it in the model.

I put it as a new more standardized workflow, but that maybe only my opinion 😃

Now one could either use the predefined path\

And there is gonna be another predefined path /fields in the current plugin.

It's only a tiny gain imho at the cost of adding a tiny bit of additional complexity

It’s not that complex, imho, the admin components just have a check if the plugins/type/name/fields exist and if so add it as a fieldpath.

The concept is simplicity, transparency and unification but to be honest I don’t know if the outcome justifies that. More options here are welcome

sovainfo · 2014-09-18T23:15:30Z

Don't understand what you meant to say. Do see that you don't want to use JPATH_COMPONENT_ADMINISTRATOR for some reason. From a programming point of view consider your code wrong. The constant JPATH_COMPONENT_ADMINISTRATOR is declared a few lines up, you have a chance to use it, but choose to recalculate it. Apart from the fact it is inconsistent it is bad programming!

sovainfo · 2014-09-18T23:33:37Z

No, use JPATH_COMPONENT_ADMINISTRATOR in both if and addFieldPath instead of calculating it twice!

dgrammatiko · 2014-09-18T23:33:49Z

@sovainfo Honestly is late, you are right I PR the change

sovainfo · 2014-09-18T23:34:47Z

tried to do that as well, can't pr your pr!

dgrammatiko · 2014-09-18T23:42:07Z

@sovainfo Did I get it right now? 😕

asika32764 · 2014-09-19T01:52:57Z

Nice change 👍

Bakual · 2014-09-19T07:39:22Z

libraries/cms/component/helper.php

@@ -322,6 +322,21 @@ public static function renderComponent($option, $params = array())
 			throw new Exception(JText::_('JLIB_APPLICATION_ERROR_COMPONENT_NOT_FOUND'), 404);
 		}

+		if (JPATH_COMPONENT == JPATH_COMPONENT_ADMINISTRATOR)


Wouldn't a simple

if (is_dir(JPATH_COMPONENT . '/models/fields')) { JForm::addFieldPath(JPATH_COMPONENT . '/models/fields'); }

do the same?

Indeed, would prefer that over the current lines 325/339

dgrammatiko · 2014-09-19T13:07:18Z

Some documentation updates needed if this gets approved.
These are the ones I found:
http://docs.joomla.org/Form_field
http://docs.joomla.org/Creating_a_custom_form_field_type
http://docs.joomla.org/Creating_a_modal_form_field

Bakual · 2014-09-19T13:55:00Z

administrator/components/com_modules/models/module.php

@@ -555,6 +555,8 @@ public function getForm($data = array(), $loadData = true)
 			$id			= JArrayHelper::getValue($data, 'id');
 		}

+		JForm::addFieldPath(JPATH_BASE . '/modules' . '/' . $module . '/fields');


That doesn't work that easy. JPATH_BASE will be pointing to admin when the module is edited in backend and to site when it is edited in frontend. At the same time we have both backend and frontend modules.
So what you need here is load the folder depending on the $clientId:

$baseFolder = ($clientId) ? JPATH_ADMINISTRATOR : JPATH_SITE; JForm::addFieldPath($baseFolder . '/modules' . '/' . $module . '/fields');

sovainfo · 2014-09-19T14:00:17Z

Disagree on removing the path existence check. Considering the amount of processing don't think it is a good idea to introduce a lot of paths that don't exist. It does take memory and cpu, why waste it?
My definition of efficient and DRY is different, I guess! It doesn't make sense to me to tell looking at places that don't even exists.
To be consistent those checks should be done for the component as well.

Bakual · 2014-09-19T14:07:05Z

Disagree on removing the path existence check. Considering the amount of processing don't think it is a good idea to introduce a lot of paths that don't exist.

I'm sure you will not be able to measure the difference. It's only a single path added. We also do a lot more such path checks for our override system, class autoloading and many more. It's not something which uses lot of ressources to process.

On the other hand, adding is_dir checks makes the code more complex and less readable.

dgrammatiko · 2014-09-19T14:24:51Z

@Bakual @sovainfo So what’s the decision here, besides the admin/front end path definition?

sovainfo · 2014-09-19T14:39:31Z

Can't take those arguments seriously. But have no interest to continue that discussion with someone I consider unreasonable.

@dgt41 Apologize for assisting you, looks like my input is not appreciated and will cause it not to be implemented. Sorry, maybe there are others that do understand the importance.

dgrammatiko · 2014-09-19T15:28:54Z

@sovainfo On the above argument I think what @Bakual says, and he is correct, is that if we do a path existence check here we are gonna repeat the procedure when we actually gonna add the fields. On the other hand the majority of modules, plugins and templates don’t use custom fields, so maybe it will be beneficial not to introduce them in the first place, and thus save some resources. Both ways are fine for me, because in either of them the harm of not doing it the other way is really marginal.

New line

Bakual · 2014-09-19T19:01:39Z

Either way will work.
Adding is_dir checks will make the code a tiny bit faster since you only check the presence of the path once instead for each field. However that is probably only measureable if you are talking about some hundred fields. In practice that will be seldom the case. Also, extension which have so many fields, most likely have at least one custom field and thus already have this path added anyway.

On the other hand it makes the code harder to read if you have

if (is_dir(JPATH_COMPONENT . '/models/forms'))
{
    JForm::addFormPath(JPATH_COMPONENT . '/models/forms');
}

if (is_dir(JPATH_COMPONENT . '/models/fields'))
{
    JForm::addFieldPath(JPATH_COMPONENT . '/models/fields'); 
}

if (is_dir(JPATH_COMPONENT . '/models/form'))
{
    JForm::addFormPath(JPATH_COMPONENT . '/model/form'); 
}

if (is_dir(JPATH_COMPONENT . '/models/field'))
{
    JForm::addFieldPath(JPATH_COMPONENT . '/model/field'); 
}

instead of

JForm::addFormPath(JPATH_COMPONENT . '/models/forms'); 
JForm::addFieldPath(JPATH_COMPONENT . '/models/fields'); 
JForm::addFormPath(JPATH_COMPONENT . '/model/form'); 
JForm::addFieldPath(JPATH_COMPONENT . '/model/field');

On a sidenote: Performance is important for pages which are shown to visitors on a regular basis. Performance doesn't really matter that much if we're talking about administrator views like we're talking here. It's not like you're going to request a module edit view several times per second.

For me, code readabilty wins hands down in this case. But it may be a personal preference. I don't care that much honestly.

dgrammatiko · 2014-09-19T20:50:01Z

I guess I’ll better leave it as is right now, till we get some more input. And if it gets some attention and successful tests I will also provide the documentation changes needed as well.

b2z · 2014-09-24T18:53:04Z

Should I apply only this PR to test it?

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

dgrammatiko · 2014-09-24T20:07:21Z

@b2z For the plugin and module it will be easier if you apply also patch #4240 because there is some custom field there so you can use it. For the templates part follow the instructions at #4295.

dgrammatiko · 2015-01-06T10:59:23Z

I will break this to three PRs with some dummy template, module, plugin to make testing easier

izharaazmi · 2015-12-17T20:53:13Z

@dgt41 @Bakual @sovainfo Why don't we make the is_dir implicit in JFormHelper::addPath() https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/helper.php#L307 and prevent adding of an invalid path in the first place.
Why are we discussing about the repetitive checks, be it anywhere!

brianteeman · 2015-12-17T21:11:56Z

Sorry replied to wrong post

dgrammatiko · 2015-12-17T21:43:26Z

@izharaazmi you got a point here!

sovainfo · 2015-12-18T08:30:26Z

@izharaazmi Sounds good to me in this case. Normally not in favor of postponing things but it looks like that is the right place to avoid adding non-existing folders to the path.
That might also be the correct place to translate /administrator. Although I would prefer the xml to be extended with an attribute for application and extending the path in case of administrator. The translation would be required still for dealing with old xml though

This should definitely be applied to J3, finally dealing with the inconsistency of suggesting the folder can be named different from administrator and hardcoding it in the rest of the software.

izharaazmi · 2016-01-13T08:59:49Z

administrator/components/com_modules/models/module.php

@@ -555,6 +555,10 @@ public function getForm($data = array(), $loadData = true)
 			$id			= JArrayHelper::getValue($data, 'id');
 		}

+		// Add the default fields directory
+		$baseFolder = ($clientId) ? JPATH_ADMINISTRATOR : JPATH_SITE;


Why don't we use JPATH_BASE here?

For modules and templates JPATH_BASE won't work as the com_modules and com_templates (in administrator) need to know if the module or template in question is front end or backend. Using JPATH_BASE will always fall back to administrator

Oh sorry, I missed that the clientId comes from the data and not the current active client.

Allow custom fileds in plugins

9bd8d53

Simple addition to allow plugins to use custom fileds in a directory named ```fields```. There shouldn't be any B/C breaks here.

jissues-bot added the PR-staging label Sep 17, 2014

copy paste missed something

43a6a59

dgrammatiko changed the title ~~Allow custom fields in plugins~~ Allow custom fields in plugins installation file [xml] Sep 17, 2014

dgrammatiko mentioned this pull request Sep 17, 2014

Introduce predefined path in templates xml file for custom fields #4295

Closed

dgrammatiko changed the title ~~Allow custom fields in plugins installation file [xml]~~ Introduce predefined path in plugins xml file for custom fields Sep 18, 2014

unified code for modules, templates, plugins

7b3f38a

dgrammatiko changed the title ~~Introduce predefined path in plugins xml file for custom fields~~ Introduce predefined path in plugins, modules, templates xml file for custom fields Sep 18, 2014

CS

1f3ebf9

dgrammatiko changed the title ~~Introduce predefined path in plugins, modules, templates xml file for custom fields~~ Introduce predefined path in plugins, modules, templates for custom fields in the xml file Sep 18, 2014

dgrammatiko added 3 commits September 18, 2014 16:50

Add same option for components

e4afc8a

CS

ae2e235

Travis is not happy

5fe84f5

dgrammatiko changed the title ~~Introduce predefined path in plugins, modules, templates for custom fields in the xml file~~ Introduce predefined path in plugins, modules, templates for custom fields in the manifest file Sep 18, 2014

dgrammatiko added 3 commits September 18, 2014 20:22

Components cannot use this one

42ee92a

Components support

df9518f

Wrong place code must move few lines below

9836bc4

dgrammatiko added 2 commits September 19, 2014 02:38

code quality

2b9a17d

Component front end

8490791

Bakual reviewed Sep 19, 2014
View reviewed changes

dgrammatiko added 2 commits September 19, 2014 19:51

Restore admin/front end check

3430b58

CS

5ef6fea

New line

dgrammatiko changed the title ~~Introduce predefined path in plugins, modules, templates for custom fields in the manifest file~~ Automate the inclusion of fields path for plugins, modules, templates Sep 24, 2014

jissues-bot changed the title ~~Automate the inclusion of fields path for plugins, modules, templates~~ Introduce predefined path in plugins, modules, templates for custom fields in the manifest file Sep 26, 2014

dgrammatiko closed this Jan 6, 2015

dgrammatiko deleted the allow_custom_fields branch August 14, 2015 16:27

dgrammatiko mentioned this pull request Dec 17, 2015

Introduce constant JADMINISTRATOR_DIR and allow renaming "administrator" folder #8721

Closed

izharaazmi reviewed Jan 13, 2016
View reviewed changes

dgrammatiko restored the allow_custom_fields branch April 6, 2016 13:13

This was referenced Apr 7, 2016

Improve templates scaffolding for custom fields #9794

Closed

Improve plugins scaffolding for custom fields #9796

Closed

Improve modules scaffolding for custom fields #9797

Closed

dgrammatiko deleted the allow_custom_fields branch April 7, 2016 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce predefined path in plugins, modules, templates for custom fields in the manifest file #4294

Introduce predefined path in plugins, modules, templates for custom fields in the manifest file #4294

dgrammatiko commented Sep 17, 2014

Bakual commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

Bakual commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

sovainfo commented Sep 18, 2014

sovainfo commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

sovainfo commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

asika32764 commented Sep 19, 2014

Bakual Sep 19, 2014

sovainfo Sep 19, 2014

dgrammatiko commented Sep 19, 2014

Bakual Sep 19, 2014

sovainfo commented Sep 19, 2014

Bakual commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

sovainfo commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

Bakual commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

b2z commented Sep 24, 2014

dgrammatiko commented Sep 24, 2014

dgrammatiko commented Jan 6, 2015

izharaazmi commented Dec 17, 2015

brianteeman commented Dec 17, 2015

dgrammatiko commented Dec 17, 2015

sovainfo commented Dec 18, 2015

izharaazmi Jan 13, 2016

dgrammatiko Apr 7, 2016

izharaazmi Apr 8, 2016

Introduce predefined path in plugins, modules, templates for custom fields in the manifest file #4294

Introduce predefined path in plugins, modules, templates for custom fields in the manifest file #4294

Conversation

dgrammatiko commented Sep 17, 2014

Simple addition to allow components, plugins, modules and templates to use custom fileds in a sub-directory named fields.

Pros

Against

There shouldn't be any B/C breaks here.

How to test

Bakual commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

Bakual commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

sovainfo commented Sep 18, 2014

sovainfo commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

sovainfo commented Sep 18, 2014

dgrammatiko commented Sep 18, 2014

asika32764 commented Sep 19, 2014

Bakual Sep 19, 2014

Choose a reason for hiding this comment

sovainfo Sep 19, 2014

Choose a reason for hiding this comment

dgrammatiko commented Sep 19, 2014

Bakual Sep 19, 2014

Choose a reason for hiding this comment

sovainfo commented Sep 19, 2014

Bakual commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

sovainfo commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

Bakual commented Sep 19, 2014

dgrammatiko commented Sep 19, 2014

b2z commented Sep 24, 2014

dgrammatiko commented Sep 24, 2014

dgrammatiko commented Jan 6, 2015

izharaazmi commented Dec 17, 2015

brianteeman commented Dec 17, 2015

dgrammatiko commented Dec 17, 2015

sovainfo commented Dec 18, 2015

izharaazmi Jan 13, 2016

Choose a reason for hiding this comment

dgrammatiko Apr 7, 2016

Choose a reason for hiding this comment

izharaazmi Apr 8, 2016

Choose a reason for hiding this comment

Simple addition to allow components, plugins, modules and templates to use custom fileds in a sub-directory named `fields`.