Selective Assets

welcome

It seems that

http://drupal.org/project/context
http://drupal.org/project/context_addassets/

provides very similar functionality. Could you describe how your module differs?

We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org.

If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module. (If the existing module is abandoned, please think about taking it over).

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also have a look at the guidelines for in-project documentation.

Also, NEVER use ?> at the end of your php files.

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
Report: http://ventral.org/pareview/httpgitdrupalorgsandboxadenot1711614git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Okay, but maybe we should spend some more time searching for a similar module, I somehow can't believe that this doesn't exist yet ;)

don't forget to switch back to needs review if your ready

I'm sorry for the delay!
There are about 100 other applications waiting for review and only a hand full of active reviewers.
We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

In your .install file you have some commented mysql code. Please, remove it.

In your .module file

include_once "includes/SelectiveAssetsAddRemove.class.inc";
include_once "includes/selective_assets.inc";

You do not need to include selective_assets.inc as long as you have 'file' element in your hook_menu()
Your class probably should be added as files[] in your .info file rather than including it here.

In your selective_assets.admin.inc you have:
'#empty' => t('No rules available. <a href="@link">Add an asset rule</a>.', array('@link' => url('admin/config/selective_assets/add'))),
I believe you should use l() function instead of a href.

 if ($pid != NULL && is_int($pid)) {
    $rule = selective_assets_load($pid);
  } 

is_int function is sufficient. If $pid is null is_int() returns false.

variable_get('clean_url', 0)
This is a user input. You should make sure it is safe. I think this is a security issue.

In your selective_assets.inc

/* print_r(drupal_get_schema('selective_assets'));exit(); */
Please, remove any commented code.

In your class file

You check for extension by substr function:

 if (substr($asset, -3) == ".js") {
          drupal_add_js($asset, 'file');

        }

PHP has its own method of checking extensions (and protocol also)
$ext = pathinfo($asset, PATHINFO_EXTENSION);

I'm still pretty new in reviewing modules, so if any of above issues is mistakenly pointed and application should not go into needs work status please, feel free to change.

Hello,

Its coding standard that you should not include any commented code in your module. There is an option to look into the table schema through some application or database itself. It is not an application blocker though.

a link was not an application blocker. Even in Drupal core there are some things that not follow coding standards. It is good though that you know something about core :)

// drupal_clear_path_cache($rule['url']); This should also be removed.

But as I said before it is not an application blocker.

I like your code, there is no security issues in my opinion and you know how to use Drupal API. It's RTBC for me :)

Very nice module and interesting use of the API !

automatic review (coder)

selective_assets.admin.inc

severity: normalLine 71: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

     '#empty' => t('No rules available.')." ".l(t('Add an asset rule'),'admin/config/selective_assets/add'),

severity: normalLine 71: missing space after comma

'#empty' => t('No rules available.')." ".l(t('Add an asset rule'),'admin/config/selective_assets/add'),

severity: normalLine 92: else statements should begin on a new line

} else {

automatic review (PAReview.sh)

There are more than 30 code standard problems.
http://ventral.org/pareview/httpgitdrupalorgsandboxadenot1711614git

manual review

1. This is vulnerable to XSS exploits. The url is user provided input and needs to be sanitized before printing into HTML. See http://drupal.org/node/28984 . Please check if there are other variables to parse before output.

Writting <script>alert('hello');</script> as the url on Home » Administration » Configuration » Selective Assets
popups an alert.

The security issue is a blocker so I set needs work again.

Concerning XSS attacks the rule is to parse the data on the output, not on the reception.
Another unsafe function could hook the value and change it with unsafe value after, then the "sanitation" you made would be broken.

Hi adenot,

1) There are still some issues at http://ventral.org/pareview/httpgitdrupalorgsandboxadenot1711614git
2) selective_assets.admin.inc, line 9: As I remember you should start include paths from DRUPAL_ROOT in Drupal 7 accordings to standard. Check that again please.
3) selective_assets.install: it's minor but I think it's better to use 'path' instead of 'url' for column name containing a path to match.
4) UI: Do you plan to have some predefined rules?
5) UI: I also think there should be an ability to enable and disable individual rules like you can enable and disable views and blocks.

Hii,

I think you cannot RTBC your own application.

Please wait form someone else to review your project and RTBC your application if everything is all right.

Also Please try to participate in Review Bonus programme and review 3 other applications. This will make this process faster and a git admin will look at your project right away.

Changing the status to needs review.

Thanks,
Aritra.

Hii Adenot,

First of all it is certainly not a requirement to review 3 projects but that does certainly accelerate the process, at least so was in my case. Considering your application is in queue for 3 months, I would certainly advise you in favour of it since it took just over 2 weeks in my case. But definitely that is not the requirement.

Secondly, after previous RTBC state,some issues were pointed out and you worked on fixing them. So again you need to change the status to needs review. When other reviewers find the changes have been appropriately made they will RTBC your application, but you yourself cannot.

Thanks,
Aritra.

Just a little correction '#empty' => t('No rules available. <a href="@link">Add an asset rule</a>.', array('@link' => url('admin/config/selective_assets/add'))), this is actually right and doing this
'#empty' => t('No rules available.')." ".l(t('Add an asset rule'),'admin/config/selective_assets/add'), is so very wrong.
For further info please consult Dynamic or static links and HTML in translatable strings

You cannot change the status of your application to RTBC as mentioned by aritra.ghosh and you cannot remove security tag.

Hi adenot!

The automated review found this:
Pareview found some issues: http://ventral.org/pareview/httpgitdrupalorgsandboxadenot1711614git-7x-1x

The errors in SelectiveAssetsAddRemove.class.inc mean that you must write comments before each function. I understand your frustration about the long review time, but this are real errors and I am afraid I must set the status to "needs work" for this. The upside is, it is easy to solve this issue :-)

The warnings in .module mean, that there is no need to document the parameters of a hook, because they are already documented in the Drupal API documentation. You should delete the @param comments.

As it is already very late now, I will finish the review for today. I will come back to you with the code review very soon.

Cheers
Martin

Hi adenot!

As I promised yesterday, here is my module test and code review:

The module configuration should become accessible through the configuration page. You should add the configuration link to one of the available categories. Therefore, in hook_menu

  $items['admin/config/selective_assets'] = array(

should become something like this:

  $items['admin/config/system/selective_assets'] = array(

When I tried to remove the style.css of bartik from a certain page, I could do it by writing style.css into the "Exclude assets" field. It did not work when I specified the whole path /themes/bartik/css/style.css with the leading "/" as you describe in your README.txt. It worked only without the leading "/". I think you should change this in your README.txt.

Apart from that the module looks fine. I read through your code and it seems very well. I think after this corrections it should be promoted to reviewed and tested by the community.

Just for your information: You wrote in #9 "The 'file' element does not support multiple files". You can use more than one files[] directives in a .info file. Look at the .info files of this core modules as examples:

modules\field\field.info
modules\openid\openid.info
modules\search\search.info
modules\system\system.info

But in your case it seems ok to use include_once for selective_assets.inc.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.