Sandbox project:

https://www.drupal.org/node/2496335

Description:

The Wrap Word module allows a user to create a text format that wraps a word (determined by position) in HTML tags.

For example:

This is a test.

can become

<span>This</span> is a test.

by specifying a span tag to wrap the first word.

The module also allows the user to specify classes for the HTML tags.

At present, the current tags are supported:

<div>
<p>
<span>

and the current positions are supported:

first
last

Clone repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/HenryW/2496335.git wrap_word
cd wrap_word

How to test

1. Install the module from admin/modules
2. Go to input format page admin/config/content/formats (Admin->configuration->text formats).
3. Select a format (e.g. Full HTML)
4. Check the checkbox next to 'Wrap Word' under 'Enabled filters"
5. Make sure the necessary default tags and positions are configured in "Wrap Word" under "Filter settings" and click 'Save configuration'.
6. Create a node with the selected format.
7. View source to see the tags for the word in the position you added (i.e. first or last).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

darol100’s picture

Status: Needs review » Needs work

Automated Review

Pareview.sh is showing some warning - http://pareview.sh/pareview/httpgitdrupalorgsandboxhenryw2496335git, No a blocker but it would be nice to be fix before the release.

Coder Review

Coder show some minors warning, still not a blocker.

wrap_word.module

  • Line 131: in most cases, replace the string function with the drupal_ equivalent string functions
            $length = strlen($selected_word[2][0][0]);
    
  • Line 137: in most cases, replace the string function with the drupal_ equivalent string functions
            $length = strlen($selected_word[2][$offset][0]);

wrap_word.helpers.inc

  • Line 65: in most cases, replace the string function with the drupal_ equivalent string functions
        $before = $dom->createTextNode(substr($text->nodeValue, 0, $offset));
  • Line 69: in most cases, replace the string function with the drupal_ equivalent string functions
        $during = $dom->createTextNode(substr($text->nodeValue, $offset, $length));
  • Line 74: in most cases, replace the string function with the drupal_ equivalent string functions
      if ($end_offset < strlen($text->nodeValue)) {
  • Line 75: in most cases, replace the string function with the drupal_ equivalent string functions
        $after = $dom->createTextNode(substr($text->nodeValue, $end_offset));

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
No: Does not follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. PAReview: 3rd party code
    DOMWordsIterator.php appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

    The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

Pravin Ajaaz’s picture

Automated Review:

Please fix the issues generated by pareview:
http://pareview.sh/pareview/httpgitdrupalorgsandboxhenryw2496335git

Manual Review:

1. You should pass the #options in t() function [$elements['tags']].

2. Please consider changing the comment block for your custom functions. /** Implements **/ usually used for hook functions.

3. Also add @param and @return for your custom functions

HenryW’s picture

Update

Many thanks to darol100 and Pravin Ajaaz for the feedback.

I have implemented the following changes:

  • strlen() to drupal_strlen()
  • substr() to drupal_substr()
  • As seen in Views module, the t function is omitted when HTML tag names are used in #options. However, the values have been updated to only show the tag name.
  • All non-hook functions will have at least one @param or @return doc block tag
  • In regards to the idea of
    DOMWordsIterator.php appears to be 3rd party code

    As a piece of throwaway code put up on this article, which doesn't have a corresponding GitHub account/repository, it seems to not be a 'maintained project' and so would best be incorporated into this module, where development is able to continue. This should be OK since the author explicitly released the code to the Public Domain. I've refactored the class to fit Drupal's coding style, pareview comments and changed the documentation on the class to indicate the origins of the code. Therefore the file will NOT be treated as 3rd party code.

HenryW’s picture

Status: Needs work » Needs review
PA robot’s picture

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Daniel.Moberly’s picture

Drupal usually uses the extension ".inc" for class files - I would suggest renaming your files to reflect that convention. Additionally, you may want to consider renaming MainTest.php to a more intuitive filename. Something like
WrapWordMainTest.inc to reflect the class it contains - MainTest is a pretty generic filename that might cause conflicts with another module somewhere along the line.

README.txt requirements - you might want to indicate that DOM is a required PHP extension. See http://php.net/manual/en/dom.setup.php

HenryW’s picture

Thanks for the feedback Daniel.Moberly.

I have implemented the following changes:

  • DOMWordsIterator.php to WrapWordDOMWordsIterator.inc
  • MainTest.php to WrapWordMainTest.php
  • In regards to the point of

    README.txt requirements - you might want to indicate that DOM is a required PHP extension. See http://php.net/manual/en/dom.setup.php

    As an already required system requirement of Drupal, I feel there is no need to include in the module requirements. See https://www.drupal.org/requirements/php#xml

Audrius Vaitonis’s picture

Status: Needs review » Needs work

Hi,

After renaming file "DOMWordsIterator.php to WrapWordDOMWordsIterator.inc" you forgot to update ".info" file

I got error "Fatal error: Class 'WrapWordDOMWordsIterator' not found in .../docroot/sites/all/modules/test/wrap_word/wrap_word.module on line 120"

Otherwise module works great.

It would be nice to have code snippet for alternative solution. You could add snippet to your theme or module and get the same simple functionality. Sometimes you don't want separate module and configurations for simple functionality.

Javascript version from "http://stackoverflow.com/questions/55612/css-to-increase-size-of-first-word"
$('#links a').each(function(){
var me = $(this);
me.html( me.text().replace(/(^\w+)/,'$1') );
});

HenryW’s picture

Status: Needs work » Needs review

Thanks for the feedback Audrius Vaitonis.

I have implemented the following changes:

  • Updated .info to reflect the name changes of WrapWordDOMWordsIterator.inc and WrapWordMainTest.php
vijaycs85’s picture

Issue summary: View changes
Status: Needs review » Needs work

Overall, the module looks very good I have installed the module and tested end to end. Here is some findings for few improvements:

1. I'm not very sure how we ship the unit test configuration, but the bootstrap.php in a module looks not right to me. we can provide unit test as you did WrapWordMainTest.php. But I don't think phpunit.xml.dist and bootstrap.php are necessary.

2. This one looks like a typo:

  public function valid() {
    return !!$this->current;
  }

3. Worth adding @see for this:

 * Based on the demonstration article by Patrick Galbraith
 * http://www.pjgalbraith.com/truncating-text-html-with-php/

4.

these are not implementations of hooks. "Callback for filter settings" or something like that would be appropriate. 
/**
 * Implements callback_filter_settings().
 */
...
/**
 * Implements callback_filter_process().
 */

5. Minor: Its very hard to read what WrapWordMainTest::data trying to provide. It's worth splitting the individual tests as separate method and data provider.

6. Project page can be updated to mention that
a) the position is specific to a field. i.e. there is no way to add tags to first word of every paragraph. It's just the first world of a field.
b) the order of the tags and positions are defined globally and can't be changed per content type/field/node.

All these can be added as 2.0 if necessary.

Updated issue summary with 'how to test' section.

jamsilver’s picture

2. Please consider changing the comment block for your custom functions. /** Implements **/ usually used for hook functions.

these are not implementations of hooks. "Callback for filter settings" or something like that would be appropriate.

Looking at the API documentation and comment standards and the example of core filter.module itself, Implements callback_NAME() seems to be the correct comment block, no?

It links in nicely to filter module's callback_filter_process API documentation.

vijaycs85’s picture

Reg #11: Didn't know it works that way. Ignore my comment on that.

HenryW’s picture

Issue summary: View changes
HenryW’s picture

Status: Needs work » Needs review

Thanks for the feedback vijaycs85!

I have implemented the following changes:

1. Regarding your comment about PHPUnit and the inclusion of bootstrap.php and phpunit.xml.dist - I felt it is entirely necessary to include these as PHPUnit is wholly dependent on them at this current time. Nevertheless, I have re-reviewed bootstrap.php and have submitted a change which will allow the PHPUnit tests to run regardless of where the module is kept within the Drupal directory.

2. Please be assured this function does not have a typo...

/**
   * Implements valid() (Iterator interface).
   */

As you will see in the above function comment, it states that the function is simply implementing valid() from http://php.net/manual/en/iterator.valid.php. As you can see on that page, it states the function valid()'s return value will be casted to boolean - by using the !! operator, we are casting $this->current to boolean FALSE.

3. @see has now been implemented for WrapWordDOMWordsIterator.inc.

4. As per comment #11 and comment #12 - this will be ignored.

5. At present, I believe the test file is well documented and the structure of the file is a trivial issue at the most.

6. As the module progresses through these stages, the project page will be overhauled to be as helpful as possible!

Prashant.c’s picture

FileSize
47.36 KB

Hi Henry,

I tested this module and it seems like there is an issue with wrapping.

I checked only DIV under HTML tags on admin/config/content/formats/full_html and created a node with "this is text" body content.

So on the node view page "this" is wrapped in a

tag where as "is text" is wrapped within

tag.

Attaching screenshot for the configurations i have used in Full HTML format.

thatpatguy’s picture

This module appears to work as intended. I think you could add a bit more documentation help explain exactly how the HTML is going to be rendered. When I first tested I was a little bit surprised that when I chose the DIV element it wasn't wrapped in the P element. This makes sense from a proper HTML hierarchy standpoint, but based on the documentation you currently have I was actually expecting the DIV to be nested inside the P element being provided by the Field/wysiwyg. Other than that this module looks good.

There are still two errors and three warnings when going through pareview.sh

FILE: /var/www/drupal-7-pareview/pareview_temp/tests/bootstrap.php
---------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
---------------------------------------------------------------------------
19 | WARNING | Line exceeds 80 characters; contains 86 characters
33 | ERROR | Use the function ip_address() instead of
| | $_SERVER['REMOTE_ADDR']
34 | ERROR | Use the function ip_address() instead of
| | $_SERVER['REMOTE_ADDR']
---------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/wrap_word.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------
67 | WARNING | #options values usually have to run through t() for
| | translation
125 | WARNING | Unused variable $word.
---------------------------------------------------------------------------

Automated Review

There are two errors and three warnings in the code:
http://pareview.sh/pareview/httpgitdrupalorgsandboxhenryw2496335git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Does follow the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements.

cllamas’s picture

FileSize
1.93 KB

I've wrote a patch to fix the code sniffer problem

PatrickScheffer’s picture

Status: Needs review » Needs work

Hi! Your module looks good, but the automated test still found some issues. No big ones, and my guess is that cllamas' patch will fix some (if not all of them), but I was a little bit shocked by the 150 issues found in tests/WrapWordMainTest.php. Apparently, the automated test does not like simple test...

Besides from that I don't really see any reason why this module should not be accepted once cllamas' patch has been applied and no other issues show up.

Here's my manual report for the record.

Manual Review

Individual user account
Not applicable.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Not applicable.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity. Nice and clean code, perhaps some extra inline documentation would be nice.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
Looking good! Maybe a little for the future: when there is only one lib or include file, it is not necessary to create a seperate folder for it. But it's not prohibited either, so it's your call.
klausi’s picture

Status: Needs work » Needs review

Those minor coding standard issues are not application blockers, anything else that you found or should this be RTBC instead?

PatrickScheffer’s picture

Status: Needs review » Reviewed & tested by the community

Allright, if no one has any objections, I'll set this to RTBC.

mlncn’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 stay 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.

Status: Fixed » Closed (fixed)

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