[D8] Menu Markup

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2782259

Project 2: https://www.drupal.org/node/2773107

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxincursus2763601git

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.

Thanks for your patience with the process incursus, and thanks for sharing your progress with it ;-)

In Drupal8 we have been starting to use https://www.drupal.org/project/special_menu_items for what sounds like a similar task to what you are up to.
It would help if you added a note about the comparative differences between that and your module on your project page. It's nice to provide a cross-link to help builders decide on which alternative suits them best.

I'm gonna try yours out locally for a quick round of feedback now ..

On first install, from the 'Extend' page, ticked "Menu Markup", Pressed save, got PHP Fatal Error.
Fatal error: Call to undefined function get_t() in {}/modules/custom/menu_markup/menu_markup.module on line 14

(Core:Drupal 8.1.7, 2016-07-18)

So, not a great start for testing. :-(

THANK YOU for adding a handy welcome message and explanation at install time, but...
Pretty sure get_t() is D7 only.

I'm seeing a whole load of whitespace and tab errors in codesniffer review. I don't know why pareview is not pointing that out today.
Running Drupal Code Standards sniffer locally I get heaps of reported problems.

The tab problems (at least) can be easily seen at http://cgit.drupalcode.org/sandbox-incursus-2763601/tree/src/Form/Config...
while http://cgit.drupalcode.org/sandbox-incursus-2763601/tree/src/Controller/... has 147 (Drupal Style) formatting problems!

Suggestion on how to fix all that (patch attached) submitted at #2784053: Code Style cleanup needed.

DrupalPractice has found some issues with your code, but could be false positives.
FILE: ...pal-7-pareview/pareview_temp/src/Controller/MenuMarkupController.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
89 | WARNING | t() calls should be avoided in classes, use dependency
| | injection and $this->t() instead
94 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
--------------------------------------------------------------------------

Automated Review

Kindly check the pareview.sh link, some trivial issue you can fix... to adhere to Drupal coding standard's
http://pareview.sh/pareview/httpsgitdrupalorgsandboxincursus2763601git

Manual Review

Individual user account

[Yes: Follows / No: Does not follow] the guidelines for individual user accounts.

No duplication

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

Master Branch

[Yes: Follows / No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows / No: Does not follow] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows / No: Does not follow] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

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. (*) Major finding, needs work
2. (+) Release blocker
3. Just a recommendation
4. ...]

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.

@aditya_anurag: Looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

@klausi @incursus,

I reviewed it again now it looks fine. So I am not changing the status.

Confirming that the automated review issue is a false positive, and that the review by @aditya_anurag that just re-iterates that mistake is failing the Turing test. We have robots to do the robot jobs, the manual reviews are for humans to perform.
Also, the copy&paste from @aditya_anurag there does not appear to have any of the template text modified or edited at all - you are supposed to have deleted the stuff that does not apply, or at least add some confirmation of what was actually looked at!

Trying it out with simplytest.me for a change..
https://simplytest.me/project/2763601

* Installed without error at /admin/modules this time.

* The install message "Congrats! menu_markup settings are available under the [configuration menu]" was helpful. You are also encouraged to add the path to config form via the "configure" key in the .info file so that you can find it directly from the module admin page again later.

* Following the test instructions at the top of this page, I visited "admin/config/menu_markup/configure" and got a "page not found" - although the breadcrumb suggested there was something near there to be found.

* I could not see the "Menu Markup" item come up under /admin/config as implied either.

* Editing the URL manually to /admin/config/menu_markup took me to a bewildering empty form.

Primitive UI/Idiosyncratic Config Tokenizer

The UI (or lack thereof) seen here is extremely un-drupally. We are supposed to key this behaviour by using the text title of a menu item? And a handmade tokenized string syntax where we have a perfectly good Drupal Form API to do this sort of task cleanly?
Given what's available in the Drupal menu API, string-parsing like this feels primitive, and is probably conceptually missing the correct way (and place) to do this... It's a concern.
A site builder who wanted to use a module like this would expect to find their options for advanced "menu markup" on the menu item edit screen, not far away in a text blob somewhere else. That sort of config blob just won't scale with use, and is very vulnerable to corruption. (I know, I've done it before, felt clever, and later regretted it)

Compare your string-tokenizer with the venerable menu_attributes project (150,000 happy customers).
This sort of precise menu item edit form is the UI that editors expect (granted there is too much whitespace in the UI here, but that's partly because I expanded everything for illustration). Clunky, but clear, maintainable, validate-able and standardized is better than terse and clever, but idiosyncratic and mysterious.

This approach makes me concerned, so let's see what's happening in the code here..

Unused, deprecated hook code

OK. Opening up menu_markup.module 8.x At the top we see a hook_menu()
Drupal8 does not use hook_menu() at all and that function there is chaff.

So we are off to another bad start...

Other code style issues

Adding docblock-style headers that are blank just to defeat the warnings about undocumented functions... defeats to purpose of code review.

/**
 *
 */

It does look like menu_markup_menu_links_discovered_alter() is the right function to be hanging your utility off, and it seems that caching should take care of the inefficiencies of a tokenizer. So that's OK there.

Digging in deeper:

Error suppression considered smelly

 if (@$this->_markupOptions[$link['title']]['nodeType']) {   

If you find yourself doing error suppression (usually a bad smell, usually avoidable) like this, it's almost always better to be explicit about what problem you were avoiding...
I presume the intent was something like the following?:

 if (!empty($this->_markupOptions[$link['title']]) && !empty($this->_markupOptions[$link['title']]['nodeType']) {   

The fact I have to presume means that the future maintainers have to guess - and that's our issue to avoid
That's just a small opinion though - not a review blocker.

Using the thing

You mention that it's necessary to clear the menu cache manually after making a change. Surely the thing to do would be to trigger a cache flush when the config form is submitted? It will only happen a few times in the lifetime of a site, and makes a big difference to whether your module seems to work at all - for support issues that would be 50% "have you flushed the cache again?" etc.

Can't get it to work?

Based on what it seemed I should be able to do, I tried entering a directive that could place a wrapper around my new clean sites 'Home' button that was currently displaying on my front page: (the one where I would normally be expecting to edit properties for at admin/structure/menu/link/standard.front_page/edit)

Home|<div class="home-button">|</div>|

(cleared cache though /admin/config/development/performance )
Didn't see any results in the markup at all.

As there is no validation happening, I can't tell if this is a syntax error, or I've got something wrong, or if the module just does not work, or if there is something else going on.
I tried positioning the Main Menu block in the sidebar. No luck.
I tried adding a sub page (node) with the menu item title "About Us" and the setting

About Us|<div class="about-button">|</div>

( and cleared cache again )

Still not seeing this have any effect on the markup.

I tried your example verbatim :

Home|<span class="fa fa-home"></span>&nbsp;&nbsp;Home

And still got no result in the markup (using DOM inspector), with the menu block showing in the main navigation area, or as a menu tree in the sidebar-first.

I've re-read your (good) instructions for "Standard usage scenario" at the top of this issue a few times, but still cannot kick any life into this demo.

Security concerns

From looking at the code where it is supposed to work, it seems there is no HTML sanitation applied before rendering.
I understand that the point of this module sorta is to allow any editor to add any un-validated markup to the page in an unexpected place ... but that's a problem that many many modules get pinged for as security vulnerabilities.

As the permission you have chosen for access to that setting is "administer site configuration" - which is admittedly "keys-to-the-castle" already, I won't complain to loudly about that - but it is perfectly possible (right now) for someone to inject XSS javascript or exploits using the UI you have provided, and that's all-to-often a reason for rejection. (Or would be if I could get it to work at all).

Verdict: not happy still

At the moment, this module is at the state where I would tell a team-member "do not ever use this, it's done wrong, and doesn't seem to even work". For a project we expect to build happily and maintain, we would be much better off investing our effort and endorsement into supporting and trialling menu_attributes in D8 or using the D8 successor Link Attributes for most of the instances where such a thing would be needed.

I know the module here is very slightly different - it allows adding arbitrary and unmoderated text and markup to links instead of extending link attributes in a way compatible with the drupal rendering pipeline - but that's as much a weakness as it is an advantage in some ways.

However, all these concerns and criticisms of mine are not absolute d.o project blockers in themselves (IMO). I don't think it's done well (for above reasons), but it mostly passes the minimum project requirements I guess.

But, it is the job of a project reviewer who has actually done a proper project review (the one in #28 is a total null-result non-review) to endorse an RTBC after some consensus is reached. Sorry, but
* As I can't fully or happily endorse it as-is
* As the "review" in #28 was useless (And now I'll go and be grumpy at that point elsewhere)
* As it's not really allowed for the author to RTBC (though I appreciate you can keep agitating for it ;)
I'm pushing the needle back a notch still, sorry.

Your options:
* Consider re-working that UI significantly
* Consider collaborating or offering enhancements to the other similar modules that offer such similar functionality. (Unfortunately, they will probably reject due to the minor markup/security issues you are potentially introducing - but they care and know the big issues at play in this space)
* Hope that another reviewer cares less about "Drupally" code and long-term maintenance than I do.
* Maybe this can be improved enough by adding help, validation, and security filters to what you've got ...

Thanks for taking the critical review in stride - it's provided to help us all learn and consider un-thought-of sides to the problem.

The dev environment I used this time was simplytest.me - an extremely handy standard for bootstrapping and tyre-kicking a Drupal module in a pristine test environment.
The link at the beginning of the review can be used to run the exact same environment I did.

I *do* appreciate that the purpose of the module is to provide unsanitized markup - I am advising that this is a sore point for security reviews and is responsible for way too many later security team alerts and advisories (though I don't wave that particular flag too hard).
What I have learned from experience is that there are almost always ways to avoid reflecting unsanitized code back to the user and that, despite the temporary annoyance, these methods often result in better, more thoughtful solutions and cleaner code, irrespective of the security factor. For that reason alone, I take professional suggestions that "you really don't want to do it that way" seriously. It's discouraged because reasons.

Case in point: markup munging because a designer thinks that

<li class="menu-item">
  <a href="/" class="is-active">
    <span class="fa fa-home"></span>&nbsp;&nbsp;Home</a>
</li>

is the very best and only way to produce your desired result.

It turns out that maybe placing a useful class in the wrapper, then using CSS for the rest would actually be a more progressive and cleaner product. And you'd be well served by learning that technique instead.

Injecting an empty, non-semantic span tag there, just to act as a formatting crutch - is the bad smell. (Not even gonna mention   ). It's not that injecting markup there is being done particularly badly, it's just that maybe you shouldn't be doing that at all.

With a little careful use of CSS, equivalent visual results could be produced from classy markup like:

<li class="menu-item with_icon with_icon--home">
  <a href="/" class="is-active">Home</a>
</li>

Existing examples like menu_icons or simple_menu_icons (d8)
... manage this task without the redundant span,
Each using combinations of contextual selectors, background-image, padding-left, etc
.. and in other cases, using CSS "::before { content:" sort of stuff can be leveraged for even more fun.

---------

This level of discussion is mostly out of scope of the technical review phase of the project application - it's just about some higher-level Markup vs content management sort of issues, so it's not *the* blocker at all. It's about some patterns of straightforwards code shortcuts (that work) vs prescient and elegant solutions (that lead to sustainable extension) .

PS, I do think the {{nodeCount}} feature as seen on your demo page is an incredibly nifty addition, and see why you thought it was cool enough to share!

I just predict that that config blob syntax is not doing it justice, and that this idea deserves to be a proper, extensible, 'menu decorator' sort of plugin - not a thing that's managed by a single, centralized, ASCII delimited list.

Markup is now configured on the menu link edit page, rather than in a config form. In fact, there is no more config form, which streamlines things considerably. Code is actually a heck of a lot slimmer now to boot.

Major job that, I'm sure! But glad you found the effort worthwhile!!

Good stuff. Sorry, I've actually been somewhat tied up with family/hospital stuff a few days, but was meaning to get back to this - given the re-work I know you've been doing.

I'll give it a kick around now. I feel totally good about what you've been doing (from your good updates here), and unless there is something mad, I think I'll be able to green-light it in a bit...

I tried updating it on my older (dirty) test site.

Not sure if it's me, by design, or unintentional, but the new UI doesn't turn up on non-node menu links (like /admin/structure/menu/link/standard.front_page/edit at least)
I expect that we will want to garnish non-node menu items like views pages just as much (if not more) than flat node ones. This may be a side effect of my testing process, so I'll try to replicate better.

It does show if I add a standard node page to the menu and edit that menu item.
However, as before, the results are not yet apparent in the markup. I kicked the cache a few times.
Rebooting with a brand new (simplytest) site now to compare..

Trialling on https://simplytest.me/project/2763601

• Enabled "Menu Markup" at /admin/modules
• Went to Menu Management to edit "main navigation" /admin/structure/menu/manage/main
• Tried to edit the "Home" menu link at /admin/structure/menu/link/standard.front_page/edit - could not.

...

• Added a node page to see if that works better. /node/add/page
  
• We don't see menu links until the menu block is shown somewhere usefully, so: placed a "Main Navigation" menu block in to "sidebar first"
  
• Given the menu now showing up, I "edit" the menu, and can add some text to the link.
  
  

This shows up! Winning!

I tried some more interesting markup - spans with classes. They showed up good also.
I tried adding some script exploits - that "worked" also.

Such is life.

To see whether the problem with not being able to edit non-node menu items is pervasive ..
I updated the default-ish '/node' view, to give us a "View" with a "Menu Item". Quite a common need.

Attempting to edit the menu item for that view page - also fails to let us edit menu markup.

Hm. Lets try some other alternatives, like creating a menu item for a taxonomy/term/n page.
Well, at least the UI shows up if I create that manually...

But oh dear ... is it true that from now on every menu item will need to go and explicitly enter @title into that collapsed div ??
Because - if I add no custom menu markup ... I get an empty menu item!

... It would seem that this has the ability to vanish all non-customized menu links on anyones existing site. That would give someone a heart attack

While you are there - please make the collapsed fieldset default to OPEN if there is customizations ... but leave it closed if the markup field is blank.
That's a common UI convention for better affordance, and will help maintainers orient themselves to what is important on that form.

OK, so it looks like it's working, and doing the job it's expected to.
Would be good to figure out if there is a reason it's not applying to programatically-provided links - but that can be a known limitation for now.
Will be necessary before a stable release to *not* make it blank out all 'normal' menu links - but that's an easy TODO.

.. Giving the code a last run-over...

I see you have started thinking about extensibility with the list of menu_markup.config:fields and the way you are serializing the settings. Smells good!
This flexible approach will serve you well in the future!

The way you save the settings in menu_markup_menu_link_content_form_submit() looks to be respectful of other modules needs, as well as your own future expansions. Yummy.

As far as I can see, the rest of the code looks not only sane, but, as you say, slimmer than before!

Note to reviewers: Although there are a couple of whitespace complaints re-introduced now (comments > 80 chars), Coding standards alone are not project application blockers. It's the first step of code-smell-checking, not the last word in good code. (That said, it's always better to just fix the damn things when they are seen than to defend the point).

Yeah, We've had enough fun here, Let's roll!

Although I was the one who pushed back from RTBC to Needs Work earlier - this work has clearly and fully been done. And although we would often prefer the one who does the RTBC *not* be the same one who promotes (unless the queue is getting full) - well, It wasn't, and it is.
;-)

Therefore I'll bump us through to the end.
You've clearly got the hang of doing things Drupally and usefully.

To keep off the security wolves in case they find you later - consider putting a decent disclaimer on the project page acknowledging that in the wrong hands, this module provides site administrators the ability to perform XSS injections against themselves. We agree on the redundancy of that issue in this case ... but let's just avoid that red flag before it happens.

(PS, that project page is starting to look better too. Do transfer those links to "similar modules" over there when you have time - it helps shoppers, and it looks like this is nicely complementary to them)

(Removing PAReview Bonus tag as complete now)
--------------------------------

Thanks for your contribution, incursus!

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:

• 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 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.

Sweet disclaimer on the project page now.

( d.o didn't let me post the thumbs-up emoji ! )