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).
Comment | File | Size | Author |
---|---|---|---|
#17 | patch_codesniffer.txt | 1.93 KB | cllamas |
#15 | Full-HTML-config.png | 47.36 KB | Prashant.c |
Comments
Comment #1
darol100 CreditAttribution: darol100 as a volunteer and commentedAutomated 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
wrap_word.helpers.inc
Manual Review
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.
Comment #2
Pravin Ajaaz CreditAttribution: Pravin Ajaaz commentedAutomated 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
Comment #3
HenryW CreditAttribution: HenryW commentedUpdate
Many thanks to darol100 and Pravin Ajaaz for the feedback.
I have implemented the following changes:
strlen()
todrupal_strlen()
substr()
todrupal_substr()
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.
Comment #4
HenryW CreditAttribution: HenryW commentedComment #5
PA robot CreditAttribution: PA robot commentedWe 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.
Comment #6
Daniel.Moberly CreditAttribution: Daniel.Moberly commentedDrupal 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
Comment #7
HenryW CreditAttribution: HenryW commentedThanks for the feedback Daniel.Moberly.
I have implemented the following changes:
DOMWordsIterator.php
toWrapWordDOMWordsIterator.inc
MainTest.php
toWrapWordMainTest.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
Comment #8
Audrius Vaitonis CreditAttribution: Audrius Vaitonis at BrightLemon commentedHi,
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') );
});
Comment #9
HenryW CreditAttribution: HenryW commentedThanks for the feedback Audrius Vaitonis.
I have implemented the following changes:
.info
to reflect the name changes ofWrapWordDOMWordsIterator.inc
andWrapWordMainTest.php
Comment #10
vijaycs85Overall, 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 thinkphpunit.xml.dist
andbootstrap.php
are necessary.2. This one looks like a typo:
3. Worth adding @see for this:
4.
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.
Comment #11
jamsilver CreditAttribution: jamsilver commentedLooking 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.Comment #12
vijaycs85Reg #11: Didn't know it works that way. Ignore my comment on that.
Comment #13
HenryW CreditAttribution: HenryW commentedComment #14
HenryW CreditAttribution: HenryW commentedThanks for the feedback vijaycs85!
I have implemented the following changes:
1. Regarding your comment about PHPUnit and the inclusion of
bootstrap.php
andphpunit.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-reviewedbootstrap.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...
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 functionvalid()
's return value will be casted to boolean - by using the!!
operator, we are casting$this->current
to booleanFALSE
.3.
@see
has now been implemented forWrapWordDOMWordsIterator.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!
Comment #15
Prashant.cHi 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.
Attaching screenshot for the configurations i have used in Full HTML format.
Comment #16
thatpatguy CreditAttribution: thatpatguy commentedThis 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
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.
Comment #17
cllamas CreditAttribution: cllamas as a volunteer commentedI've wrote a patch to fix the code sniffer problem
Comment #18
PatrickScheffer CreditAttribution: PatrickScheffer commentedHi! 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
Comment #19
klausiThose minor coding standard issues are not application blockers, anything else that you found or should this be RTBC instead?
Comment #20
PatrickScheffer CreditAttribution: PatrickScheffer commentedAllright, if no one has any objections, I'll set this to RTBC.
Comment #21
mlncn CreditAttribution: mlncn at Agaric commentedThanks 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.