Name: Markdown Insert
Core: 7.x
Project Page: https://drupal.org/sandbox/skh/2271863
Git: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/skh/2271863.git markdown_insert
Review:http://pareview.sh/pareview/httpgitdrupalorgsandboxskh2271863git

Description

The Insert module outputs HTML, but when using Markdown syntax (text format), it is preferable to have this output in Markdown for consistency.

This module alters the output of the Insert module to use Markdown syntax for image_image and file_generic widget types.

Reviews

https://drupal.org/node/2278193#comment-8836857
https://drupal.org/node/2284061#comment-8867541
https://www.drupal.org/node/2298389#comment-8946001

Comments

skh’s picture

Issue summary: View changes
lonalore’s picture

Status: Needs review » Needs work

Hi skh,

Your module seems to be an error and did not passed pareview test see this link:
http://pareview.sh/pareview/httpgitdrupalorgsandboxskh2271863git

And I noticed that, there is a typo in your .install file. The single quote mark is after LIMIT 1, and not after markdown_insert.

db_query("
    UPDATE {system}
    SET weight = 20
    WHERE type = 'module' AND
          name = 'markdown_insert
    LIMIT 1'
  ");
skh’s picture

Status: Needs work » Needs review

Thank you for the review. I have updated as per your remarks, and re-ran the tests at http://pareview.sh/pareview/httpgitdrupalorgsandboxskh2271863git

skh’s picture

Issue summary: View changes

Git branch name changed

dimple_chandra’s picture

Please add README.txt
Kindly mention in the description what is different in this module?

skh’s picture

@dimple_chandra: Thanks for the review. However, there already is a README.txt, and I think the description states rather clearly it's intended purpose. Please let me know if there is something unclear. The module quite literally just alters the insert module to output markdown syntax instead of HTML.

dimple_chandra’s picture

I cloned the module but i could not find README.txt

skh’s picture

http://cgit.drupalcode.org/sandbox-skh-2271863/tree/README.txt

Seems to be there? Not sure why it isn't there for you, but if you believe it's something on my end please let me know!

Thanks.

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.

dimple_chandra’s picture

Thanks @skh i found the README.txt in your tree link but its strange while cloning i can't see that .txt

keopx’s picture

Use README.txt official template https://drupal.org/node/2181737, because very difficult read it.

skh’s picture

Okay. I think that's rather subjective, but I have moved that readme to a .md file (in the hopes of 1674976), and added a txt.

skh’s picture

Issue summary: View changes
skh’s picture

Priority: Normal » Major

Needs review state for 15 days as per 539608.

skh’s picture

Issue summary: View changes

Added review.

keopx’s picture

Issue summary: View changes

@skh read about Review bonus to have more visibility.

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

skh’s picture

Issue summary: View changes
skh’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +PAreview: review bonus
  • Added review link
  • PAReview tag
  • Changed priority as per 539608
keopx’s picture

Status: Needs review » Needs work

Hi change this format, example: Implements of hook_form_FORM_ID_alter(). to
Implements hook_form_FORM_ID_alter(). Without of

You have errors on some files, see: http://pareview.sh/pareview/httpgitdrupalorgsandboxskh2271863git

er.pushpinderrana’s picture

@skh, your module looks good and I also installed the same on my machine. I also reviewed code and get confused with following query.

I am not sure why are you using LIMIT here because you have already added where condition with name then I think there is no need to use limit. It is just increasing extra overhead on query and need to be fixed.

db_query("
    UPDATE {system}
    SET weight = 20
    WHERE type = 'module' AND
          name = 'markdown_insert'
    LIMIT 1
  ");

Rest looks fine.

Thanks!

skh’s picture

klausi’s picture

Assigned: Unassigned » dman
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/markdown_insert.js
    --------------------------------------------------------------------------------
    FOUND 2 ERRORS AFFECTING 2 LINES
    --------------------------------------------------------------------------------
      5 | ERROR | Doc comment short description must be on a single line, further
        |       | text should be a separate paragraph
     26 | ERROR | Inline comments must end in full-stops, exclamation marks, or
        |       | question marks
    --------------------------------------------------------------------------------
    
  • DrupalPractice has found some issues with your code, but could be false positives.
    FILE: /home/klausi/pareview_temp/markdown_insert.install
    --------------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    --------------------------------------------------------------------------------
     15 | WARNING | Do not use UPDATE queries with db_query(), use db_update()
        |         | instead
    --------------------------------------------------------------------------------
    

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. I think it is not necessary to have both a README.md and a README.txt file - remove one of them.
  2. markdown_insert_install(): do not juggle with module weights as that is unreliable, use hook_module_implements_alter() instead if you need to run before/after a specific hook.
  3. markdown_insert_field_widget_info_alter(): the variable_get('markdown_insert', FALSE) is never set, where does it come from? Please add a comment.
  4. markdown_insert_process_field(): do not use drupal_add_js() here, add the JS with the #attached attribute to the $element instead. See https://www.drupal.org/node/756722

Although you should definitely fix those issues they are not absolute critical application blockers, so I guess this is RTBC otherwise. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to take a final look at this.

skh’s picture

Thank-you for the review. I have completed the revisions as suggested, with one note:

markdown_insert_install(): do not juggle with module weights as that is unreliable, use hook_module_implements_alter() instead if you need to run before/after a specific hook.

I did it this way because the insert module creates the hook, and the implementation is not found otherwise. They also seemed to do it the same way.

I also noticed a few other popular modules like views, admin_menu, and devel did the same thing.

I was thinking of just updating it to module_set_weight when creating a D8 version (like devel). For now, I've changed it to use db_update.

dman’s picture

Status: Reviewed & tested by the community » Fixed

User testing:
Pretty good project page. Setup was straightforward, the setting was where it said it would be.

I enabled the modules, and added a 'markdown' text format with the markdown filter enabled. No wysiwyg of course, if we are choosing markdown.

Configured the field settings:
Enable insert button
Enable Markdown Insert
Chose 'link to file, and 'medium' as the default.

Added a page with an image to add. Pressed the button and It inserted a fugly blob:

[![](/sites/embedder.drupal7.gizmo/files/styles/medium/public/field/image/Photo%2024.jpg?itok=TGEEEeNB)](<a href="/sites/embedder.drupal7.gizmo/files/field/image/Photo%2024.jpg" title="__description__">__description_or_filename__</a>)

... this ugliness is why I have given up on markdown and tokenizers when HTML would do better... *sigh*
But - it's exactly what I expected from this sort of module. So does it work?

Well, I *do* get the img rendered, but I get a load of crud around it also. Is that supposed to be happening?
If I disable the markdown_insert and just use normal (HTML) insert, it's all fine. If I re-enable it again, I get crud.
... the crud goes away when I disable 'link to file' option from insert module.
So, that's a bug, but one that can be fixed as a project issue.

I was also able to trivially break it by inserting "dodgy ] text [ into th' description" - so you should probably sanitize that better.

I would have maybe approached either the insert.module team to see if this option could be added as just a patch, but as it's a 'glue' module, I guess it's OK stand-alone.

Anyway, code looks fine and readable. it does what it should, using the right tools. Plenty of reviews above have been done and passed.
This can be green-lighted. With a few small things to look at as new issues for you.

------------------

Thanks for your contribution, skh!

I updated your account so you can 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 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.

Thanks to the dedicated reviewer(s) as well.

skh’s picture

Thank-you to dman and everyone else for the great suggestions and help.

dman, you have inspired me to make markdown in Drupal more accessible and user-friendly. I have a couple of other markdown tools in development, and now have plans to add an option for reference-style links and potentially a simple WYSIWYG-style formatting toolbar.

Thanks again!
Sam.

Status: Fixed » Closed (fixed)

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