Assetic

You have in your module 3rd party script (i mean yuicompressor-2.4.7.jar)
Please read 3rd party libraries on Drupal.org. You probably have to remove it from the module directory.

This is what the Coder says:

assetic.module

    severity: minorLine 99: @see references should be separated by "," followed by a single space and with no trailing punctuation

     * @see hook_assetic_filters()

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

        } else {

    severity: minorLine 179: @see references should be separated by "," followed by a single space and with no trailing punctuation

     * @see assetic_element_info_alter()

    severity: minorLine 180: @see references should be separated by "," followed by a single space and with no trailing punctuation

     * @see drupal_pre_render_styles()

    severity: criticalLine 222: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)

        drupal_set_message(t('The Assetic library isn\'t correctly installed. Please check the !status for more information.', array(

    severity: minorLine 234: @see references should be separated by "," followed by a single space and with no trailing punctuation

     * @see assetic_libraries_info()

Hide sites/all/modules/assetic/assetic.api.php
assetic.api.php

    severity: minorLine 12: @see references should be separated by "," followed by a single space and with no trailing punctuation

     * @see hook_assetic_filters_info()

Actually I used the Coder on my local PC. I rechecked the code using another tool, some issues still here

there is another method to generate urls, you can try

$t('Please download the Assetic library from <a href="@url">@url</url>.', array(
   '@url' => $assetic_library['download url'], // for internal paths : url($assetic_library['download url'])

here is my review:

• some minor issues reported here:
  Automatic PAReview.sh

README.txt

• line 13: Make sure that the Assetic files are existing at 'sites/all/assetic'.
  

  you mean 'sites/all/libraries/assetic', right?

assetic.module

• line 77: function less_cron_queue_info() {
  should be:
  function assetic_cron_queue_info() {
• line 271: 'description' => 'Administer the Assetic settings',
  shouldn't the text be enclosed in t()?
• line 347: in the t() function, the @-signs html-escape the link, which shows it as plain text, not link. you should use '!status' in that, and in the next line's too. you should probably check your other t() calls for similar issue. (to test this, just rename you libraries/assets to something else, and view the module's config page.)

assetic.install:

• you should delete the 'assetic_debug' and 'assetic_cache_directory' variables, in your hook_uninstall function.

Hi robinvdvleuten,

There is no need to use t() inside t() on line 265 in file assetic.module.

Thanks,
Udaksh

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hello,

Automated Review:

There are some minor issues at:
http://ventral.org/pareview/httpgitdrupalorgsandboxrobinvdvleuten1826144git

Manual Review:

1. There are various empty functions such as assetic_drush_dump() and assetic_compass_assetic_filter_compass_alter() are you planning on adding code to them?

2. One suggestion would be to use an includes folder to store includes for good tidy practice (As I like to think).

I also suggest you follow up klausi's post so you can get an in-depth review and get the experience of reviewing other projects.

There are stiil some warnings at:
http://ventral.org/pareview/httpgitdrupalorgsandboxrobinvdvleuten1826144git

You should not document your hooks with "Implements hook_foo()." in an *.api.php file. That is the actual hook documentation, so you should describe what the hook does and what it can be used for. Example: http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/sy...

Hello,

- You ought to change admin.inc to assetic_admin_form.inc or something similar for more clarity as it's not self explanatary as it made me think of the admin module and it is standard practice to using the module name as prefix for all files.
- There is no contents to the assetic_compass_form function in the assetic_compass.admin.inc file meaning there is just an empty configuration form.

Hi.

First time reviewer, not quite sure what to do… Here goes:

Automated Review

Did not do.

Manual Review

** assetic.install, line 61

There is, probably, no need to use get_t() inside of _assetic_requirements_check_library(). You sad it yourself: "We can only check for library dependencies on runtime." So it is probably safe to use t() directly.

But then again, what about updates? Should these checks be run when DB is being updated?

** assetic.install, line 98

I am quite sure that there is no need to translate modules' names. I am refering to this part:

l($t("X Autoload"), 'http://drupal.org/project/xautoload')

Look into system_modules() function, you will find a lot of code like this, in which module name is passed as parameter to t():

t('@module (<span class="admin-missing">missing</span>)', array('@module' => drupal_ucfirst($requires)))

** assetic.install, line 83

Check the order of parameters in a call to version_compare(). For example, your code requires assetic of version 1.1.0.

version_compare('1.1.0', '1.1.0') == 0  /* branch will not be executed */
version_compare('1.1.0', '1.5.0') == -1 /* branch WILL be executed */
version_compare('1.1.0', '0.9.0') == 1  /* branch will not be executed */

So, basically, your code accepts the exact version of the library OR any earlier version. But it will produce a requirements error if a user uses any newer version, even if it is a simple minor update (v. 1.1.1). This behavior is opposite to my expectations.

** assetic.module, lines 126 and 154

Correct me if I'm wrong, but I understand that on line 154 you try to get path to a folder where cached assets will be stored (i.e. public://assetic/). And on line 126 you try to delete everything from the same folder. The problem here is the call to file_build_uri(), which in turn calls file_default_scheme(). So, in fact, your cached assets may be stored in private://assetic/ and it will never be cleared.

Update 1:

** assetic.install, line 29; assetic.module, line 211

Why do you create new cache_assetic table and not use Drupal's generic cache table? It seems to me that your module writes only one single record into it (assetic.module:211). Tables are not expensive, but still this is an overkill.

** assetic.module, line 105

Small typo in function doc: its should be it's.

Update 2: The rest of the code looks good.

Update 3: I have installed the module and set the default download method to private and all styles totally broke. Drupal denies access to private files by default and your module does not implement hook_file_download() to grant access.

Since I have already debugged this issue localy, here is my commit. It will remove calls to file_build_uri() and always save assets in public dir.

Maybe my explanations were not clear enough, sorry about that. Again, in file assetic.install, line 79, the correct operator should be >, that way it will accept version 1.1.0 and any never versions. Use my first patch.

If you return 'cache' from assetic_flush_cache(), then cache table will be cleared twice. You do not need to return anything. Use my second patch.

This module is an early alpha and a lot of its functionality has not been implemented yet, but it works. It successfully transformed my SCSS-stylesheets into CSS, the code looks fine to me and I would change its status to RTBC, but unfortunately this module does not work with core's CSS aggregation. I think, it is an important feature and it should be supported.

Also, this module needs a better installation instructions. It took me some time to figure out that Symfony/Process library must be placed in a symfony-process directory.

But both of these improvements can be done after project is approved. So I am going to leave the issue's status unchanged, so that one more reviewer can take a look at the code.

manual review:

1. assetic.api.php: examples would be nice :-)
2. "variable_get('assetic_cache_directory', NULL),": that variable is not removed in hook_uninstall()?
3. assetic_compass_admin_form(): empty admin page? No @todo what should be implemented here, so why do you need it?
4. assetic_drush_dump(): empty function, so the command does not work? Please add a @todo.
5. README.txt: how does the module work? What do I have to do as developer with it? Same for the project page.

But that are not blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Yup, I can confirm that aggregation works. Thanks for addressing the issue that fast!

The main module and both sub-modules now have new installation and usage documentation (a total of 444 new words in README files). And I see that the usage of version_compare() is now correct :)

This module is still being reviewed, so here are few more things I noticed:

** file assetic_compass.api.php, line 13

If this hook accepts an object, there is no need to pass it by reference. http://php.net/manual/en/language.oop5.references.php

** file assetic.info, line 7

This page does not exist anymore :)

** README files

There is a small number of minor typos in README files, like extra 's' in verbs.

Three hooks still do not have usage examples, but now module is in a much better shape, a lot of dead code has been cleaned.

So, klausi, what is your final word on this module? :)

Well, I can't fix applications that I RTBC'ed myself so fast. I need another person to confirm the RTBC, so do you think it is ready? If I don't here from you or any complaints within a week I'm going to approve this. Or a different git admin comes along and approves it before that if she/he agrees on the RTBC.

@klausi
Well, yes, to my taste this module is RTBC. I cannot find any obvious bugs anymore, or think of any trivial improvements. This module is quite ready to be tested on live projects.

@robinvdvleuten
You still have to write hook usage examples. :) And an open question to you:

What is going to happen if all three of the sub-modules are activated at once? One of them is going to win and solely process files? Or styles will be sequentially processed by all three filters?

Thanks for your contribution, robinvdvleuten!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

@robinvdvleuten come around the core queue, we could use your knowledge #1751602: Use Assetic to package JS and CSS files