Support for the Assetic asset management framework in Drupal.
With this module you can use Assetic as asset management tool in Drupal. So you can use frontend development tools like SASS, Compass, LESS and many more .

The purpose of this module is to have a solid foundation for the base functionality of Assetic, so other modules can add filters through the use of hooks.

The sandbox can be found here;
http://drupal.org/node/1826144
And the repository can be found here;
http://git.drupal.org/sandbox/robinvdvleuten/1826144.git

Thanks in advance for reviewing!

Reviews of other projects:

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

ymakux’s picture

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.

robinvdvleuten’s picture

Hi ymakux,

The 3rd party script is removed. Do you maybe have other things that can be improved?

ymakux’s picture

Status: Needs review » Needs work

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()

robinvdvleuten’s picture

Status: Needs work » Needs review

I ran the PAReview tool on the module and do not get any usable error messages. Only one about a l() function call, but this is correct because I am using a variable (the url of a repository) as title argument so this shouldn't be translated.

ymakux’s picture

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

robinvdvleuten’s picture

The issue given by PAReview isn't really an issue;

FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/assetic.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
63 | ERROR | The $text argument to l() should be enclosed within t() so that
| | it is translatable
--------------------------------------------------------------------------------

It's about the following line in my install script;

$requirements['assetic_library']['description'] = $t('Please download the Assetic library from !url.', array(
   '!url' => l($assetic_library['download url'], $assetic_library['download url']),
));

That variable shouldn't be wrapped in a t() function because it's a full url and shouldn't be translated. If you now a better way for generating an url with the url as title let me know :)

ymakux’s picture

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'])

robinvdvleuten’s picture

I doubt if this is really better than using the l() function, but I will implement it :)

herom’s picture

Status: Needs review » Needs work

here is my review:

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.
robinvdvleuten’s picture

Status: Needs work » Needs review

I've fixed all issues that appeared in the PAReview report:
Automatic PAreview

Abou the t() function in hook_menu, this is unnecessary as described here. It says that the title and description should be untranslated.

All of your other comments are fixed, like the incorrect named hook and the link in the status message.

udaksh’s picture

Hi robinvdvleuten,

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

Thanks,
Udaksh

klausi’s picture

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 :-)

fr3shw3b’s picture

Status: Needs review » Needs work

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.

robinvdvleuten’s picture

Status: Needs work » Needs review

I moved the include of the admin interface functionality to a separate folder. The drush include needs to be in the root directory with this particular name for the drush integration to work. The empty drush function is an improvement for the near future, but the other one was just there as an example and is moved to the api.php file with hooks.
The PAReview issues are issues about the docblock comments in the api.php files. I do not know exactly how to fix them and they appear harmless to me :) I will try to find some time to review other projects soon.

monymirza’s picture

Status: Needs review » Needs work
robinvdvleuten’s picture

These warnings are a bit confusing for me and are about the docblock annotations. If someone can help me with these, I'll fix them. But because they are about comments, I do not see any issues by not fixing.

klausi’s picture

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...

robinvdvleuten’s picture

Status: Needs work » Needs review

I've fixed all issues that appeared in the PAReview;
http://ventral.org/pareview/httpgitdrupalorgsandboxrobinvdvleuten1826144git

fr3shw3b’s picture

Status: Needs review » Needs work

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.

robinvdvleuten’s picture

Issue summary: View changes

Reviewed another project.

robinvdvleuten’s picture

Issue summary: View changes

Added a review of another project.

robinvdvleuten’s picture

Status: Needs work » Needs review

I do not see the point of transforming the name and path to what it was. I guess everyone has a different opinion about how to add includes inside a module's file structure.

robinvdvleuten’s picture

Issue tags: +PAReview: review bonus

Reviewed three other projects, so I add the review bonus tag.

20th’s picture

Status: Needs review » Needs work

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.

20th’s picture

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.

robinvdvleuten’s picture

Status: Needs work » Needs review

Thanks 20th for such an in-depth analysis! I've applied the patch for the public directory path and incorporated your suggestions.
There is now only one issue in the automated review about using the t() function, but because it's the name of the module this is probably unnecessary (http://ventral.org/pareview/httpgitdrupalorgsandboxrobinvdvleuten1826144git).

20th’s picture

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.

robinvdvleuten’s picture

Are you sure about the operator? I've checked the documentation's example and they use >= to compare a version that should be at least PHP 5.3.0 (http://php.net/version_compare).
The module is still indeed in an early alpha state and hopefully I will get some help from the community for creating a solid module that adds Assetic's functionality to Drupal. I will dive into the core's CSS aggregation as this is a very useful and must-have feature of the module.

Thanks again for such an in-depth analysis of my module :)

klausi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -PAReview: review bonus

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.

robinvdvleuten’s picture

I've added a better description on how to use the module. All unnecessary code for the admin menu hooks are removed.
The status is now changed to 'reviewed and tested by the community', but how is this sandbox now granted to a project?

robinvdvleuten’s picture

Issue summary: View changes

Added a review of another project.

robinvdvleuten’s picture

Issue tags: +PAReview: review bonus

Reviewed three new projects, so I added the review bonus tag.

robinvdvleuten’s picture

@20th The module now also plays nicely with Drupal core's CSS aggregation :)

20th’s picture

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? :)

klausi’s picture

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.

robinvdvleuten’s picture

I have fixed the last issues mentioned by 20th :)

20th’s picture

@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?

klausi’s picture

Status: Reviewed & tested by the community » Fixed

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:

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’s picture

Thanks 20th, klausi and all others for reviewing my module :)

nod_’s picture

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

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added the reviews of three other projects.