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

[iamntz]

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.

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

[iamntz]

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.

[tw2113]

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

[boonebgorges]

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 :)

[iamntz]

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!

[boonebgorges]

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 :)