Save config XMLs to site files directory, not module directory.

@jastern,

This does seem to be a somewhat common issue so I'll look into adding it as part of the Readme.

Thanks for the suggestion!

a better solution would be to automatically create a folder in the Drupal site's files folder. Then these xml files could be referenced from sites/default/files/jwplayer/configs (for example). On install it could copy it's own sample config xml file into that folder so that one still exists by default. This would be much more conforming with the Drupal way of doing things and cause people to have to deal with file permissions less often.

1. As @btopro suggests, please use Drupal's files directory. It is not good security practice to allow modules to write to their own directories (or any location containing executable PHP). The correct place to write files is in the site's files directory.

2. In jwplayermodule_admin() you should check for success of the config XML write before alerting of the successful save. Otherwise you end up with an alert message from JWPlayer Admin telling the user the save was successful, and behind that an error message from Drupal telling the user that actually, no it wasn't -

http://drupal.org/files/jwplayermodule-853346-cognitive-dissonance.png

@grobot,

1. Yes, this issue came up in the submission review process. For the initial release of the module it was considered acceptable since the configs are being saved to a subdirectory which contains no code and can have it's own permissions set. However, going forward I will make the effort to shift the save location to Drupal's files directory.

2. This is a bug and will be fixed in an upcoming release.

Thanks for the feedback guys. It's definitely appreciated.

Just hit this issue again during an installation. Here's a patch which resolves the issue (for me), although I can't claim to have tested it extensively.

This adds a LongTailVideoFramework::getConfigDir() method which attempts to provide a jwplayer_configs directory inside file_directory_path().

I tested config load/edit/save/copy/delete and using the saved config. All seems to work fine.

Note that this will have a behavioural change (improvement!). Multisite setups using JWPlayer presumably share all Player configs across all sites when the module is installed in sites/all/modules. With this fix, each site can have separate configurations without the need to install a separate copy of the code.

grobot,

Nice work on an initial patch. I have tested the patch and it works as described, but I thought I would bring up a couple of issues before we commit this.

1. The patch does not adhere to Drupal coding standards, see http://drupal.org/coding-standards.
2. Rather than having a single configuration directory, I think a better solution would be to provide a Drupal hook, say hook_jwplayer_config, that would allow other modules to specify jwplayer configurations. The jwplayer module can include the sample configuration found in the modules 'configs' directory and also any configurations that are found in a site specific configuration folder. The configurations would need to be prefixed with the module/theme/site name to avoid duplicates.
3. Because the same method in step 2 could be used to allow other modules to extend the jwplayer module via skins and plugins, the jwplayer module should create a directory structure like 'jwpalyer/configs', 'jwplayer/plugins' and 'jwplayer/skins' in the sites files folder rather than 'jwplayer_config'.

Just my two cents. I am happy to help by testing any new patches.

Cheers,

Antoine

@AntoineSolutions,

I like the idea of using a theme function to handle the configs, plugins and skins. That would make the module way more extensible. I'll definitely look into this for a future update.

Thanks.

@AntoineSolutions, thanks for alerting me to the trailing space in the comments which my patch introduced at line 131. That was a close one ;)

Other than that, I don't see any coding standards issues which the patch introduces. I checked the module before and after using coder module set to 'minor'. If you can elaborate on any other issues introduced or in lines which this patch affects, I'd be happy to update again.

@grobot,

Besides the tailing space that would have been devastating if entered into source control ;), I did see a couple other issues that caused me to raise a flag:
- Control structures should not have a space between opening/closing parentheses and their contents.
- There are several issues with your Doxygen comment for the new getConfigDir() function. See http://drupal.org/node/1354#functions for more information.

There may be other issues, but I don't have time to go through and point them all out. Coding standards are periodically updated and should be reviewed often. I only raise the issue because it's important to me that all Drupal code be in unison as much as possible making it much easier to understand and extend.

Cheers,

Antoine

Thanks Jon, appreciate the detail. Hope this does the trick!

Hey grobot,

Looking better, but a couple other things I see.

1. All text in .txt files should be wrapped at 80 characters.
2. The new getConfigDir() function is so easily described, you can probably take advantage of the summary only Doxygen comment style with something like the following:
   

   /*
    * Returns the path for player configs directory, creating if appropriate.
    */
   

3. Review http://drupal.org/node/1354#inline
4. In several commands you have a space between the command and the ending semicolon. Although I don't believe there is a standard for this, I have never seen it before and think it would be more appropriate to remove any spacing between commands and the ending semicolon.

With the above corrections, I think this patch will be ready to be committed and is a good first step towards the other goals I outlined in #6.

Cheers,

Antoine