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

Support a 'basedir' attribute in filelist and folderlist #20344

Closed

Conversation

okonomiyaki3000
Copy link
Contributor

@okonomiyaki3000 okonomiyaki3000 commented May 11, 2018

Summary of Changes

Added support for a basedir attribute in JFormFieldList and JFormFieldFolderList and JFormFieldFileList. This allows the field to be configured such that the directory attribute is relative to one of several directories which can't be known ahead of time since their values can be changed by the user. Specifically, these are the JPATH_ constants and parameter values from various extension types.

Testing Instructions

Add a filelist or folderlist type field to any jform. Notice what happens when the field has a basedir attribute which may be set to any of:

  • JPATH_ROOT
  • JPATH_SITE
  • JPATH_CONFIGURATION
  • JPATH_ADMINISTRATION
  • JPATH_LIBRARIES
  • JPATH_PLUGINS
  • JPATH_INSTALLATION
  • JPATH_THEMES
  • JPATH_CACHE
  • JPATH_MANIFESTS
  • file_path
  • image_path
  • The value of any component param (ex: component:com_media:file_path)
  • The value of any library param (ex: library:joomla:param_name, note: Joomla libraries do not actually have params)
  • The value of any module param (ex: module:mod_menu:some_menu_title:param_name)
  • The value of any plugin param (ex: plugin:system:myplugin:param_name)

So, here's a pretty real test:
Go to the media manager and change the location of the files and images directories.
Go to the plugin settings for 'Fields - Imagelist' plugin.
This plugin is supposed to show a list of directories in the images directory but it will not be your new custom images directory, it will always just list the contents of /images which isn't very useful.
With this patch, that field can be configured with basedir="component:com_media:image_path" and directory="/" and it should show the correct contents.

Expected result

Without any basedir attribute, the directory attribute will be relative to the root of the Joomla intallation. With a basedir, it will be relative to that directory.

Actual result

All good.

Documentation Changes Required

Maybe a little.

@okonomiyaki3000
Copy link
Contributor Author

One thing I'm not so sure about here is file_path and image_path. Maybe they would be better as com_media:file_path and com_media:image_path so that such values could be handled in a more generic way instead of as special cases. Based on such a value we could get the params for any component and use any of its values as the base dir.

I guess it could even be better than that. We could have component:com_media:file_path and component:com_media:image_path which would basically mean that we might also use other extension types. Something like plugin:plg_whatever:some_param could be used. Is that just getting to be too much? Or would it be useful? I think not difficult at all to develop in any case.

@okonomiyaki3000
Copy link
Contributor Author

By the way, this drone failure is nonsense.

*/
protected function getBaseDir($basedir = '')
{
// Basedir is any of the JPATH constants, resolve it
Copy link
Contributor

Choose a reason for hiding this comment

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

How about checking for empty value first?

		if ($basedir === '')
		{
			return '';
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I guess it couldn't hurt.

@ggppdk
Copy link
Contributor

ggppdk commented May 17, 2018

I guess it could even be better than that. We could have component:com_media:file_path and component:com_media:image_path which would basically mean that we might also use other extension types. Something like plugin:plg_whatever:some_param could be used. Is that just getting to be too much? Or would it be useful? I think not difficult at all to develop in any case.

Yes better not introduced 2 hard coded cases for com media

please add it as you suggested component:com_media:file_path,
and i will help test this

You can just add support for component parameters now
and if needed some other PR can add for plugins and modules parameters in the future

if you add for plugin it should also include folder too
plugin:search:mysearch:some_path

$ops = explode(':', $basedir);
if ($ops[0] === 'plugin' && count($ops) === 5)
{
  $plugin = JPlugin::getPlugin($ops[1], $ops[2]);
  $pluginParams = new JRegistry($plugin ? $plugin->params : null);
  ...
}

@laoneo
Copy link
Member

laoneo commented May 17, 2018

The filelist and folderlist fields do not have a connection to com_media and should not. If you want to make one which depends on com_media then you need to create your own mediafolderlist form field.

@okonomiyaki3000
Copy link
Contributor Author

I've removed any reference to any specific component and used the more generalized method as discussed. You can get a basedir value from the params of any component, plugin, library (I know, right?), or module (OK, this one is kinda funny).

I also noticed that there are some standard joomla plugins that use this type of field with directory set to 'images'. This is obviously a problem if the user changes his images directory to something else. So those plugins are probably currently broken if you have a custom images directory. With this patch, we can fix them.

* Method to resolve the optional base directory as one of several configuration-dependent values
* The base dir can be any of the defined 'JPATH_' constants or one of the directories configured by com_media
*
* @param string $basedir The name of one of the 'JPATH_' constants, 'file_path', 'image_path', or empty
Copy link
Contributor

Choose a reason for hiding this comment

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

'file_path', 'image_path', are no longer applicable.

@okonomiyaki3000
Copy link
Contributor Author

It seems to me that these two classes are 90% identical code. Could be that they should both inherit from some abstract 'filesystemitemlist' class or something like that. In fact, maybe that's not even necessary. filelist could probably inherit from folderlist and just override two or three functions. Am I wrong?

Traits could also be a solution except that they aren't available before php 5.4.

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@laoneo
Copy link
Member

laoneo commented Mar 25, 2022

Sorry that it took so long to respond. This is here is a nice feature, unfortunately it should be added to Joomla 4. Can you rebase the pr to the 4.2-dev branch, so we can test it properly. In the meantime I'm closing the pr, when ready please reopen again. Thanks for your contribution making Joomla better.

@laoneo laoneo closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants