Imagefield CSS

Hi,

README.txt is empty. can it have some description?

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

Two minor comments:
- Please add new line to end of imagefield_css.info file
- Please check that README.txt file is in utf8 format
(See: Maintainer: Alexandre Isra�l )

your git link is wrong.
It should be: git clone http://git.drupal.org/sandbox/w3wfr/1860060.git imagefield_css

installing the module:
When I went to the module page to turn the module on I got these errors:

Notice: Undefined index: name in system_sort_modules_by_info_name() (line 944 of C:\wamp\www\d7\modules\system\system.admin.inc).
Notice: Undefined index: name in _system_modules_build_row() (line 977 of C:\wamp\www\d7\modules\system\system.admin.inc).
Notice: Undefined index: name in system_modules() (line 901 of C:\wamp\www\d7\modules\system\system.admin.inc).

I tried checking out your code 3 times with the same result.
I then recreated the file and copy and pasted the contents of the original and saved, and the errors went away.

I then did the automated review and this is actually reported:
http://ventral.org/pareview/httpgitdrupalorgsandboxw3wfr1860060git

Installation went ok after fixing the file.

Using the module:
After enabling the module I put an image field on the basic page content type and set the image field to css declaration. When the settings for the field appeared I was surprised to see a 'selector' textfield with 'body' as the default value. I assumed this module would replace the image tag with a div or something with the image set as the background image, at this stage I was a little confused.
Anyway, I left all the default values and saved the field settings. I then created a basic page and the image was set as a background to the body tag.

Although this module did not do what I thought it would do (just output a div with a background image), It does fulfill a requirement which I have often needed for client sites (being able to set a background image on a per page basis).

Maybe this should be outlined better in the readme. I see that there are examples/use cases in the readme but I think it needs to clearly state that the field will not be output at all. Maybe it's just my comprehension skills. Maybe someone else can give their thoughts on this.

This is a module which I would definitely use.

Code:
The code looks ok to me. Though I'm not sure if you need to send the setting inputs though a function to sanitize such as check_plain in theme_imagefield_css_formatter or if this is already done by using token_replace.

Feature request:
I would like to see some css3 settings such as background-size

Hi w3wfr,

Just wanted to report I also encountered the problem raised by 2pha at #6 and fixed it as indicated:
Copy pasted image_field.info in a blank txt file that I renamed image_field.info, then it worked fine.

On top of the previous comments, I would like to add the following ones:

1 - Project page
Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.
Screenshots could also be a good idea to make things easier to understand for some users.

2 - XSS Security issue:
Thanks to grisendo at #1837170: Image Link Formatter, #3, I learnt how easy/often this could happen.
The problem with these types of issues is that it's by essence invisible or hard to think of when you're coding.

So I've tested the issue, manually and, indeed:
If you try to enter <script>alert('XSS');</script> as the value of the textfields Color, Horizontal position or Vertical position, of the Imagefield CSS formatter form settings, in the display page (in the summary) the JS code is executed.

To prevent these types of issues from happening, it is recommended to add a call to filter_xss_admin before instantiating the values in the summary.
So for example in current module's code, in imagefield_css.module, line 172:
$summary[] = t('Color') . ': ' . $settings['color'];
would have to be changed to:
$summary[] = t('Color') . ': ' . filter_xss_admin($settings['color']);

From what I understood, the same could apply with any textfield that would be provided in any display formatter summary on the Manage Display page.
[So these fixes would also have to be applied in imagefield_css.module at lines 182, from what I saw, but you'd better test and double check again]

In any case, if you have a doubt, simply try the test above:
I tried it manually on current module's version and it did, as expected, execute the JS code (Alert window XSS).

Therefore, I allowed myself to add the tag PAReview: security.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

(Quoted from: #1837170: Image Link Formatter, #10)

One last validation related advice would be for you to run the coding standards validation (PAReview) again, after you have committed your changes, to make sure you didn't introduce any new coding standards errors when you made the changes. After that, feel free to change the status back to needs review, that will save you one not so useful review from another user who would have simply told you about the coding errors mistakes (more back and forth = more time). This is something very common that happens to all of us (and just for one whitespace at the end of the line... the status is changed back to needs work again).

So far, that's all I could find, but I think we should be giving this project another round of reviews after all comments reported above (including #6) have been fixed.

Feel free to let me know if you would have any other questions, comments or concerns on these items, I would surely be glad to provide more information.
Your module looks good and has a pretty cool functionality. Well done! and I hope these comments will help you to improving it.

Cheers!

Hi Alexandre, this is a neat little module, thanks for posting!

- Installed module ok
- Successfully changed the Image Field format
- Repeat values and horizontal position values worked ok
- Image styles worked ok
- Vertical and Horizontal positions worked ok

1. I, too, was a bit thrown off by the "body" default value for the Selector field. Perhaps you could document this a bit more explicitly in the installation instructions with a "NOTE:" or something? Or even have a more descriptive default value inserted directly into the form field, like "Change this to a valid CSS selector" (which it says in the help text below, so that's helpful).

2. Would you consider the Token module as a dependency or an optional feature? I first enabled the Imagefield CSS module without Token installed and the CSS Declaration form shows "TOKENS" dropdown with no tokens to choose from (see attached image). Maybe you can add some logic around that form element declaration to check if the Token module is enabled?

Cool, I replicated the testing steps listed above in my previous comment in a clean D7 installation and it appeared to work ok.

I see the notice in the README and Drush successfully downloaded and enabled the Token module when I enabled this one.

Looks good to me!

// Jason

Thanks for your contribution, Alexandre!

Using "body" as default value for the selector is not really good, since it does not work out-of-the-box ("body" does not match any element on a default Drupal page). It would be better to use ".field-name-body" instead.

But as this is nothing that would block your application 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.