Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#36832 closed enhancement (maybelater)

Refactor for class-walker-nav-menu-edit.php

Reported by: iamntz's profile iamntz Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.6
Component: Taxonomy Keywords: has-patch
Focuses: administration Cc:

Description

Hi guys.

I present you a small refactor of the class-walker-nav-menu-edit.php class. Reasoning:

  • The start_el method is huge;
  • The start_el method have a lot of responsibilities;
  • The walker is impossible to extend without duplicating a lot of code (basically the whole start_el method)

The idea behind this refactor:

  • reduce responsibilities for the main method;
  • adding a hook that allows extending fields (so will close #18584);
  • adding a hook that allows extending title & controls;

So, what did i do?

  • I added a hook for: title, controls and settings fields;
  • I extracted everything into its own method: title, submenu status, controls, settings;
  • I extracted most conditionals into its own methods;
  • All methods are now easier to follow (under 20 lines each)

Additional work

Depending on how this patch will be received, I'm planning to do a similar refactor for Walker_nav_menu, because it kind of have the same issues with the start_el method.

Attachments (3)

class-walker-nav-menu-edit.diff (22.2 KB) - added by iamntz 8 years ago.
class-walker-nav-menu-edit.2.diff (22.2 KB) - added by iamntz 8 years ago.
class-walker-nav-menu-edit.3.diff (22.6 KB) - added by iamntz 8 years ago.

Download all attachments as: .zip

Change History (12)

#1 @iamntz
8 years ago

  • Keywords has-patch added

#2 follow-up: @tw2113
8 years ago

I could see potential issue here, though not guaranteed, for anyone who's extended those classes. Especially if breaking things up into smaller methods for multiple parts instead of all being part of one/small handful of methods.

#3 in reply to: ↑ 2 @iamntz
8 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Replying to tw2113:

I could see potential issue here, though not guaranteed, for anyone who's extended those classes. Especially if breaking things up into smaller methods for multiple parts instead of all being part of one/small handful of methods.

Hmmm, you're right. I didn't take in account that case. I'll adjust the patch to not add the default hooks on construct, but a bit later, inside of start_el method.

#4 @iamntz
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#5 @tw2113
8 years ago

Good to hear you're not dismayed and will instead refactor approach for consideration.

#6 @boonebgorges
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Hi @iamntz - Thanks for the patch.

We have a policy of not doing major refactoring, except when the refactor is necessary to fix bugs or to enable certain kinds of use cases. It looks to me like the refactoring you're suggesting doesn't enable anything fundamentally new, or fix any bugs, so I'm afraid it's probably off the table without further justification. See https://make.wordpress.org/core/handbook/contribute/code-refactoring/ for more details.

Out of curiosity, what is the advantage of using add_action() to build various parts of the UI, rather than simply calling the methods (eg $this->get_menu_item_fields_url_markup())?

If you'd like to propose a fix for #18584, I'd suggest proposing it on that ticket. I haven't waded through that ticket's history, but I imagine there's a reason why it hasn't been resolved yet :)

#7 @iamntz
8 years ago

Hey Boone, thanks for reviewing.

The reason for using add action instead calling the methods is simple: it provides a bigger flexibility (i.e. what if you want to add a custom field before default fields? Do it my way and you only to change priority of the custom hook; what if you want - for some crazy reason - to remove all fields?) and reduces responsibilities for the main method.

Additionally, unit tests could be written more easily, without depending on too many stuff.


About refactoring: this patch, besides enabling couple of extra hooks, will be more friendly in case you need to extend the Walker. Instead of copying a 100 lines method to change something trivial, you only need to copy few lines tops.

I wasn't aware of this policy of not refactoring.

Thanks again!

#8 @boonebgorges
8 years ago

  • Component changed from Menus to Taxonomy

The reason for using add action instead calling the methods is simple: it provides a bigger flexibility

Gotcha. It makes sense to try to be as flexible as reasonably possible. But if you only goal is to put things before and after the core-provided markup, it would be just as easy to extend the core class with a method that looks like this:

    public function get_menu_item_title_markup( ... ) {
        parent::get_menu_item_title_markup( ... );

        // My extra stuff goes here
    }

A hook-based solution that would provide even greater flexibility is if the markup was built and then passed through a filter. This would allow the dev to add stuff to the beginning or end of the markup, or to do on-the-fly modification of the markup generated by core.

I'm changing the status of this ticket to maybelater. I can see a way forward for a systematic rethink of the way that the Walker classes build markup, but it'll have to look something like this:

  1. Do an analysis of all core Walker classes, and come up with a naming convention and method design that can be shared as much as possible across the classes. It may make sense for this to be an interface rather than a base class.
  2. Review the various ways that Walker classes are being overridden in real-world applications, and show (a) how backward compatibility will be maintained even after the refactor, and/or (b) how and why backward compatibility will be broken.
  3. Review open, wontfix, and maybelater Walker related tickets currently in Trac - #18584 is a good place to start - and see if it's possible to address a number of these tickets as part of a redesign. This will help build a case that a refactor is worth the effort.
  4. Ensure that there's decent test coverage for the existing Walker classes. We generally test these classes indirectly, through integration tests for the wrapper functions (eg, tests/phpunit/tests/category/wpListCategories.php for Walker_Category). Additional test coverage can be introduced a bit at a time, and bits of tests can be submitted at any time through standalone enhancement tickets.

Thanks for taking an interest :)

#9 @boonebgorges
8 years ago

  • Resolution changed from wontfix to maybelater
Note: See TracTickets for help on using tickets.