The Field Menu module provides a field type that allows the selection of a menu tree to display in page.
This is useful if for example you want a really customized sitemap. Simply apply it to a content type or Paragraph entity so you can output your own, dynamic, sitemap page with minimal configuration.

Project link

https://www.drupal.org/project/field_menu

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/field_menu.git

My reviews

  1. https://www.drupal.org/project/projectapplications/issues/3169658

Comments

code-brighton created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
mrweiner’s picture

mrweiner commented
Status: Needs review » Needs work

Checklist

  • Individual user account: yes
  • No duplication: Yes, as far as I can tell
  • Master Branch: yes, uses 8.x-1.x naming
  • Licensing: yes, no licensing needed
  • 3rd party assets: yes, no 3rd party assets
  • README: no, see additional notes
  • Code long/complex enough for review: yes
  • Secure code: yes
  • Coding style & Drupal API usage: no, see below

Additional notes

  • The README and module page do describe the module, but the usage instructions could be more clear. The "configuration" section says to add a field, but provide the field type. It was unclear to me at first that I needed to select the "Menu Item" field type.
  • Also just a usability note, it would help to wrap the elements in TreeWidget.php in a container/fieldset so that the fields are better grouped and separated from other fields on the form.

Coding style & Drupal API usage

There are a lot of coding standards issues. I'd recommend getting https://www.drupal.org/project/coder set up and then run the Drupal and DrupalPractice checks. Among the problems are

  • t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  • \Drupal calls should be avoided in classes, use dependency injection instead. Here's an example of how to do that for a FieldFormatter
code-brighton’s picture

code-brighton commented

@mrweiner thank you so much for your comprehensive review!

This is excellent stuff. I think I've resolved all the issues you highlighted now in a new release 8.x-1.0-alpha6

Many thanks :)

code-brighton’s picture

code-brighton commented
Status: Needs work » Needs review

Updating status to needs review as issues raised I believe been addressed in 8.x-1.0-alpha6 (and latest dev)

mrweiner’s picture

mrweiner commented
Status: Needs review » Needs work

Looking good! Looks like your latest dev branch is throwing an error, though. ParseError: syntax error, unexpected ')' in Drupal\Core\Extension\Extension->load() (line 39 of modules/contrib/field_menu/field_menu.module). Minor issue, just has a trailing comma where one isn't allowed. After that's fixed I think this is probably good.

code-brighton’s picture

code-brighton commented

Ah great, thanks you :) I've fixe that trailing comma issue and pushed to dev...strange thing is I couldn't get my local system to throw an error? Tried all PHP error settings, I'm running PHP 7.4 ...maybe version thing? but I agree shouldn't have been there...just odd that I couldn't get the error. Thanks for highlighting. Good to go?

code-brighton’s picture

code-brighton commented
Status: Needs work » Needs review
mrweiner’s picture

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

Ah yeah probably php version -- I guess I've got my test env running 7.2 at the moment. Things look good to me but I'm new 'round this queue so I guess we'll see!

https://www.drupal.org/node/894256#anchor-workflow doesn't mention marking as RTBTC after review is done but...I'm assuming that's the right way to do this?

code-brighton’s picture

code-brighton commented

Hey I'm a bit new to this too. I'm doing a review of another module now though...been inspired :)

Thanks! That seems like the next step? I'll see what happens next. Really appreciate your help

code-brighton’s picture

code-brighton commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

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