A text format filter that floats images left and right automatically.
It wraps images in a 'span' tag with an odd/even class and makes them float with CSS.
Details on the project page.

git clone http://git.drupal.org/sandbox/lolandese/1675006.git autofloat

Drupal version: 7.x
Project page: http://drupal.org/sandbox/lolandese/1675006

Reviews of other projects:

Without and with AutoFloat

CommentFileSizeAuthor
#22 CHANGELOG.txt3.09 KBlolandese
#12 CHANGELOG.txt2.34 KBlolandese
#8 CHANGELOG.txt1.51 KBlolandese

Comments

drupaldrop’s picture

drupaldrop commented

Hello,
For now work on the below report , I will review the code and post the comments .
Error report : http://ventral.org/pareview/httpgitdrupalorgsandboxlolandese1675006git

lolandese’s picture

lolandese commented

Thanks for your quick reply.

  • Changes suggested by the PAReview have been made.
  • The review has already been repeated and comes out 100% clean now.

Some things that are next:

  • Adding a horizontal tab in the Filter settings section under /admin/config/content/formats/full_html. Because there is only one settings page, it will redirect, giving a message like: "AutoFloat Filter settings are shared by all the input formats where it is enabled.". At least that's how other modules solve it.
  • Extensive testing and fine-tuning of the regex used:
    • For example desired is the possibility to treat also a table as an image, to include it in the alternating floating pattern.
    • Find out better the limitations and what the consequenses are. The filter is expected to break when using deep nested elements. If it proofs that the regex code used can't handle HTML sufficient, using an XML-parser instead will be considered.

Thanks again for your interest.

lolandese’s picture

lolandese commented

Links with info that probably should be applied to the code:

drupaldrop’s picture

drupaldrop commented
  • Ventral report looks good.
  • But coder still showing some error in minor. this is what i am getting from coder.

    • " Coder found 1 projects, 3 files, 2 critical warnings, 5 minor warnings, 0 warnings were flagged to be ignored "

  • Manual code review looks ok to me apart from coder issues.
  • You can improve the .info file , Follow this link http://drupal.org/node/542202.
lolandese’s picture

lolandese commented

Coder set to "minor (most sensitive)" gives me:
"Coder found 1 projects, 3 files, 0 warnings were flagged to be ignored".
Did you grab the latest git? Please, add an attachment with the report you get.

I won't use a 'package' in the .info file:

  • The documentation says not to use it unless "your module comes with other modules or is meant to be used exclusively with other modules".
  • Other filter modules sometimes are under the package 'Filters' (e.g. Custom Filter), others under 'Input filters' (e.g. Image Resize Filter), indicating there is no consensus despite the existence of a wiki page on the subject..

A 'Configure' link will be added to the .info file on the next commit, to make the settings accessible from the modules overview page.
Thanks.

For anyone else doing a manual code review, there is likely improvement possible of the used regex (preg_replace_callback, line 82 of the module file) regarding nested elements. It consist of four parts (divided by '|') matching a rejected 'div', a selected 'span', a link with image or an image. Also the parsing goes in that order, making that a pattern inside an already found pattern gets ignored, to avoid floats inside floats.

Furthermore I haven't been able to split out and prepare the pattern parts of the regex separately (better for readability):

  • Some variables declared outside the function preg_replace_callback seem not to get recognized. Using $GLOBALS gives an 'undefined index' notice. Remains a mystery why the selector and rejector variables used do get through.
  • For escapes (\) to be passed on from a variable, they must be escaped as well. Logical, but it results in the variable not being exactly as the matching pattern. To get '\\' in the pattern it must be '\\\\\' in the variable. Not really a readability improvement and that would have been our aim in the first place for splitting it out.

These are the reasons that the regex is still an ugly long line of code, but at least a working one. Any hints on how to handle this better are more than welcome.

I would also be grateful for some specific hints how to use an XML-parser instead of a regex for this case. It would allow to add a class to the existing element instead of wrapping it in a 'span'. I looked into it but couldn't form a clear idea on how to do that. Adding a dependency on the QueryPath module would be okay if that should be the way to go.
EDIT: I received some nice hints here.

lolandese’s picture

lolandese commented

After some consideration using an XML-parser instead of a regex is a hard requirement to make this project a more serious one. It is more challenging to implement but I want to give it a try.

I'm leaving it in "needs review" status as the code is functional and think sufficient to judge if basic Drupal API proficiency is present.

sanchi.girotra’s picture

sanchi.girotra commented
Status: Needs review » Needs work

Manual Review:
1.In .install file can use db_delete to delete all the variable starting with "autofloat_%".
2.In autofloat.admin, no need to write autofloat_admin_settings_submit() as return system_settings_form($form); will accordingly save content in the variable table.
3.In .module file 

   $path = drupal_get_path('module', 'autofloat');
if (variable_get('autofloat_css', 1)) {
  drupal_add_css($path . '/autofloat.css');
}

add this code in a function, can add in hook_init().
4.In .module file access argument "administer site configuration" doesn't fit in this scenario.

lolandese’s picture

lolandese commented
StatusFileSize
new CHANGELOG.txt1.51 KB

Committed suggestions of comments 3, 4 and 7. See CHANGELOG.txt for details.

Thanks for the reviews so far.

lolandese’s picture

lolandese commented
Status: Needs work » Needs review
dwieeb’s picture

dwieeb commented

Hi,

  • Your drupal_add_css() function should have the 'every_page' flag option set to TRUE for optimization of front-end performance. See drupal_add_css().
  • I do this out of habit too, but some of the longer strings in your code are enclosed by double quotes, and they don't need to be. Change them to single quotes to improve back-end performance. (So PHP doesn't interpolate them).
  • While yes, the regex is ugly and long, I don't have too much of a problem with it, considering the alternative is including a large library for navigating the generated DOM, which is an expensive process.
  • You should use the l() function to get rid of the markup in this t() function:
      $elements['notice'] = array(
    '#markup' => t('<a href="@settings">AutoFloat Filter settings</a> are shared by all the text formats where it is enabled.', array('@settings' => url('admin/config/content/autofloat'))),
  );
  • In your CSS files, not every theme will have a .content div, particularly if they implement their own .tpl files. Consider generalizing your CSS.
  • In your .install file, consider using placeholders instead of variables. Even though those particular variables won't change, it's still inserting markup directly into the translatable string, and it's best to limit the amount of markup in the first parameter of t() functions. Use placeholders and the l() function instead.
  • In your .install file, if you're going to edit the variable table directly when uninstalling, you should clear the variable cache like variable_del() does. Just put in:
    cache_clear_all('variables', 'cache_bootstrap');
dwieeb’s picture

dwieeb commented
Status: Needs review » Needs work
lolandese’s picture

lolandese commented
Status: Needs work » Needs review
StatusFileSize
new CHANGELOG.txt2.34 KB

Committed all suggestions of comment 10. See CHANGELOG.txt for details.

Thanks for again an excellent review.

P:S.: Letting go of #6 as hard requirement. Besides the mentioned performance concern and library dependency of using an XML-parser:

  • it makes the code at least twice as big and three times as complex
  • adding a span wrapper can hardly be considered HTML manipulation. It can't have dramatic consequences even if something went wrong.
lord_of_freaks’s picture

lord_of_freaks commented

Hello

Review autofloat.admin.inc beacuse of drupal_set_message after return statement

...


  return system_settings_form($form);
  drupal_set_message(t('The settings have been saved successfully.'));
}

IMHO #6 suggestion turn module code into "cleaner" code but i´m agree with @lolandese becuase it increment a lot the complexity.

Great job

lord_of_freaks’s picture

lord_of_freaks commented
Status: Needs review » Needs work
lolandese’s picture

lolandese commented
Status: Reviewed & tested by the community » Needs review

The drupal_set_message() could indeed be removed because:

  • it could never be called after a return statement
  • Drupal takes care of displaying a message itself already after saving the settings.

Committed. Thanks.

lord_of_freaks’s picture

lord_of_freaks commented
Status: Needs work » Reviewed & tested by the community
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

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

lolandese’s picture

lolandese commented
Issue tags: +PAreview: review bonus

.

lolandese’s picture

lolandese commented
Issue summary: View changes

Added "Reviews of other projects" section.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

This looks like a duplicate of http://drupal.org/project/float_filter . Module duplication and fragmentation is a huge problem on drupal.org. We prefer collaboration over competition, so please open an issue in the float_filter issue queue to discuss this and what you would have to add. You should also get in contact with the maintainer(s) to work together on float_filter.

If that fails for whatever reason please get back to us and set this back to "needs review".

lolandese’s picture

lolandese commented
Status: Postponed (maintainer needs more info) » Needs review

Ran into Float filter in search of the functionality, automatic floating of images, but it doesn't provide this. The sole aim of Float filter is to target inline styled elements with CSS. Making images float is still something that requires input from a user on each and every image through the GUI of a WYSIWYG editor or with HTML.

AutoFloat offers mainly a time-saver functionality by making images float automatically. Avoiding inline styling (not just detecting) is only a welcome side effect.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Hm ok, I think that might justify a different module. Please add the differences to float_filter to your project page.

manual review:

  1. autofloat_install(): no user provided input is involved here, so filter_xss_admin() is useless. Please read http://drupal.org/node/28984 again.
  2. autofloat_install(): do not concatenate variables into translatable strings, use placeholders with t() instead. Same in _autofloat_filter_settings().

Although you should definitively fix those issues they are no blockers, so this looks RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Mentioned project names instead of link url in "Reviews" section.

lolandese’s picture

lolandese commented
Issue tags: +PAreview: review bonus
StatusFileSize
new CHANGELOG.txt3.09 KB

Committed all suggestions of comment #21. See CHANGELOG.txt for details.

Thanks for the review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, lolandese!

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.

lolandese’s picture

lolandese commented

Thanks to all. I learned a lot. It's almost a pity you can only use the application process once. :)

My contribution efforts will remain patching, writing documentation and also reviewing from now on. Nevertheless it's nice having delivered a project that offers a functionality that is unique among 18,233 other modules.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added more reviews.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: AutoFloat » [D7] AutoFloat
Issue summary: View changes