Description

This module makes it easy for a Commerce administrator to see which node or nodes display a particular Commerce product.

The module appends a new property to the Commerce Product entity that lists the IDs of nodes that are configured to reference the product, alters the default Commerce module 'Products' view (admin/structure/views/view/commerce_products/edit), adding the new field, and also adds a fieldset listing links to any product display node(s) to the product edit page (admin/commerce/products/<product_id>).

As the Commerce administrator reconfigures their products, product reference fields and bundles, the module will update itself to keep track of these changes meaning the only configuration required by the user is to decide where the information this module makes available, is displayed.

Screen shots are available on the project page.

Project page

https://drupal.org/sandbox/alex_sansom/2143063

Git repository

git clone http://git.drupal.org/sandbox/alex_sansom/2143063.git commerce_prodnodelink

Reviews to other project applications

[D7] Commerce BluePay Hosted Forms
[D7] Commerce Checkout Register
[D7]commerce_intuit

Comments

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

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

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.

alex_sansom’s picture

alex_sansom commented

Fixed issue with PAReview complaining about missing t() for config form #options.

The remaining PAReview warnings seem, to me, to conflict with how Views field handler classes are coded. PARreview says:

FILE: ...views/handlers/commerce_prodnodelink_handler_field_display_node_ids.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
11 | ERROR | Class name must begin with a capital letter
11 | ERROR | Class name must use UpperCamel naming without underscores
21 | ERROR | Public method name
| | "commerce_prodnodelink_handler_field_display_node_ids::get_value"
| | is not in lowerCamel format, it must not contain underscores
28 | ERROR | Public method name
| | "commerce_prodnodelink_handler_field_display_node_ids::pre_render"
| | is not in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

But, looking at the way the Views module implements its classes, which is the style I've used, it's not the same as PAReview recommends.

Is anyone able to confirm which is the correct style? I couldn't find a definitive document.

alex_sansom’s picture

alex_sansom commented
Status: Needs work » Needs review
alex_sansom’s picture

alex_sansom commented
Issue tags: +PAreview: review bonus
candotri’s picture

candotri commented

I have limited experience with Views but from a general standpoint this code is easy to read, well documented, and flows nicely.

I noticed that other implementations of hook_entity_property_info_alter ( for example, entity/modules/node.info.inc) translate 'description' and 'label'). entity.api.php provides guidance and includes a flag 'translatable' but it's not clear how they intend this to be used. I think i'd t() those to be consistent unless I'm wrong on my interpretation. This is super picky but if you feel strongly you can set the 'translatable' flag to be perfectly clear.

Some reviewers prefer that the .module file is as small as possible. If possible, move functions out into an .inc. It's already quite small so I'd think the performance benefit would be small.

hook_uninstall should call variable_del() rather than manually manipulating 'variable' with sql. This is, in fact, illustrated in the api documentation for hook_uninstall. I like the watchdog message following the deletes.

Good work!

alex_sansom’s picture

alex_sansom commented

Thanks for the review Candotri, much appreciated!

With regards to your comments, I've:

Added the missing t() to the 'label' and 'description' array key values in the hook_entity_property_info alter implementation. I can see that many of the core Commerce modules (commerce_cart, commerce_order_ui etc) do this. They also do not provide the 'translatable' flag you've mentioned. I'm as unsure as you about how that should be used too. For now at least, I'll leave that off.

I can appreciate the point of view about making the module file as small as possible, therefore I've implemented hook_hook_info() and moved the feld related hook implementations into their own *.field.inc file. Probably only a small potential gain, as you say, but it can't hurt.

I've also updated the hook_uninstall() code to use variable_del() too.

Again, thanks for taking the time to review and your suggestions.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » stborchert
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  1. I think your info file misses a dependency to commerce_product?
  2. commerce_prodnodelink_entity_property_info_alter(): type is wrong, this is a list of integers, right? Then type should be "list". You could even specify it as "list", that depends if you want the entity metadata API to auto-load nodes for you or not.
  3. commerce_prodnodelink_product_display_node_ids(): could you add a comment why this does not respect the node_access tag for queries against the node table? See also https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac...
  4. commerce_prodnodelink_form_commerce_product_ui_product_form_alter(): 'Node @nid' does not tell me much, better diesplay the node title + the ID?
  5. commerce_prodnodelink_form_commerce_product_ui_product_form_alter(): do not call theme() here, just add to the nested form render array. Drupal will render it later for you.
  6. OK, so commerce_prodnodelink_form_commerce_product_ui_product_form_alter() does not respect node access and commerce_prodnodelink_product_display_node_ids() does not respect node access. I don't think this is a security issue, since no sensitive data of the node is displayed. As soon as you would display the title here you must take node access into account. I guess the same thing applies to commerce_prodnodelink_handler_field_display_node_ids.inc.
  7. commerce_prodnodelink_field_attach_delete_bundle(): why do you use preg_match() and not strpos()?

But otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

alex_sansom’s picture

alex_sansom commented

Thanks for taking time to look and comment on this project Klausi.

I've made a few more commits to address some of the points you've made above.

I think your info file misses a dependency to commerce_product?

I think you're right. I've updated the .info file dependencies to 'commerce_product'. That, indirectly, means there is a requirement of 'commerce' too.

commerce_prodnodelink_product_display_node_ids(): could you add a comment why this does not respect the node_access tag for queries against the node table? See also ...

Thanks for the documentation link. It mentions: "When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access".". So I've now done that.

commerce_prodnodelink_form_commerce_product_ui_product_form_alter(): do not call theme() here, just add to the nested form render array. Drupal will render it later for you.

If I'm just adding to the form array, I have to add it as a 'type' of form control, currently markup. If I do not call theme() here, the array is never rendered ('Array' is output to the page). Looking at the other available form item types, none appear to be output (like 'markup'), they're input controls such as radios or selects, hence I'm generating the markup and then adding it to the form.

I think I may be misunderstanding what you mean here? If I am, could you expand on what you mean, please?

OK, so commerce_prodnodelink_form_commerce_product_ui_product_form_alter() does not respect node access and commerce_prodnodelink_product_display_node_ids() does not respect node access. I don't think this is a security issue, since no sensitive data of the node is displayed. As soon as you would display the title here you must take node access into account. I guess the same thing applies to commerce_prodnodelink_handler_field_display_node_ids.inc.

After reading the 'Node access rights' link you posted, as mentioned, I've added the 'node_access' tag to the query in commerce_prodnodelink_product_display_node_ids(). Hopefully that addresses this, if I choose to display the node title in a future change.

commerce_prodnodelink_field_attach_delete_bundle(): why do you use preg_match() and not strpos()?

Good point, thanks. I've updated this to use strpos(). Testing this also lead to me fixing a small bug on bundle deletion, cheers!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, alex_sansom!

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.

alex_sansom’s picture

alex_sansom commented

no objections for more than a week, so ...

Thanks for your contribution, alex_sansom!

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.

Cool! Thanks very much Klausi and also candotri for all your help.

Status: Fixed » Closed (fixed)

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