Hi guys,

I just wanted to report a small/minor issue related with the install file of the module.

Upon uninstall, it seems module is not deleting the block_node_type database records related with menu_block.
Basically, we would need to add in menu_block_uninstall (menu_block.install line 10), another call to db_delete('block_node_type').

As a side note, I would assume perhaps a reason why this wouldn't have already been implemented would be because menu_block_uninstall would be a direct port from the 6.x-2.x branch, and since the block_node_type was only added in drupal-7.x, it didn't exist for the 6.x version.

Additionally, I would like to suggest replacing all the calls to variable_del, with a single db_delete with a 'menu_block_% wildcard for the field in the condition.
I guess this change could also be applied to the 6.x-2.x branch, see menu_block.install.

I would greatly appreciate to have your feedback on this task, and if you could let me know if I overlooked or missed anything in module's implementation, or the Drupal API in general.
Feel free to let me know if you would have any questions, comments or concerns on any aspects of the discussed issues, I would be glad to explain in more details.

Thanks very much to all, in advance, for your comments, feedback, and reporting.
Cheers!

Comments

DYdave’s picture

Quick follow up on this ticket.

Please find attached to this comment two patch files against existing supported branches:

These two patches have been tested and seem to work as expected.

For both versions, the calls to variable_del have been replaced with a single call to db_delete (7.x-2.x) or db_query (6.x-2.x).

For the 7.x-2.x version, an additional db_delete is required to remove menu_block data from the block_node_type database table.

This change doesn't add any new feature or function to the module, however, it improves its maintainability, readability or code organization.

Please let me know if you would have any questions, objections, comments, suggestions, recommendations or concerns on the patch files or any aspects discussed in this ticket, I would be glad to provide more information, explain in more details or re-roll the patches if necessary.

I would greatly appreciate some help from module maintainers and if any of you could take a bit of time to look into any of the attached patches (6.x-2.x or 7.x-2.x) to give me your feedback/opinion on this ticket.

Any questions, feedback, testing, changes, recommendations would be highly appreciated.
Thanks to all in advance.

Dave Reid’s picture

Changing all the variable_del() calls warrants more discussion than the block_node_type deletion. Can we split these into two different issues, because I would be more than ok committing the block_node_type table deletions.

  • Dave Reid committed 468d289 on 7.x-2.x
    Issue #1959312 by DYdave, Dave Reid: Fixed block_node_type table records...
Dave Reid’s picture

Status:Needs review» Fixed

Resolved in 7.x-2.x now.

Dave Reid’s picture

Category:Task» Bug report

  • Dave Reid committed 0d6bebb on 7.x-2.x
    Issue #1959312: Ensure record removal works even if block module is...

  • Dave Reid committed 0d6bebb on 7.x-3.x
    Issue #1959312: Ensure record removal works even if block module is...
  • Dave Reid committed 468d289 on 7.x-3.x
    Issue #1959312 by DYdave, Dave Reid: Fixed block_node_type table records...

Status:Fixed» Closed (fixed)

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