Adding theme support to duplicate an existing menu, export menus as custom links relative urls and import theme settings with menus #195
Conversation
…ustom links relative urls and import theme settings with menus
…u assignments if they exist
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive navigation menu management capabilities to the Memberlite theme, including a new admin interface for menu creation and duplication, plus import/export functionality with cross-environment portability through relative URL conversion.
Changes:
- New "Custom Menus" admin page under Memberlite menu for creating, duplicating, and managing navigation menus
- Enhanced theme export/import tools to support navigation menus with portable relative URLs
- UI improvements including scrollable checkbox lists, clickable rows, and consistent delete button styling
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| inc/admin.php | Adds Custom Menus submenu page and "Manage with Memberlite" quick link on Appearance > Menus page |
| functions.php | Includes new menus.php admin page file |
| css/admin.css | Adds styles for scrollable checkbox boxes and clickable row elements |
| adminpages/tools/import-settings.php | Adds menu import options UI with "Replace existing menus" checkbox |
| adminpages/tools/export-settings.php | Adds menu export options with selectable menu checkboxes and JavaScript toggle functionality |
| adminpages/tools.php | Implements menu export logic with relative URL conversion and import logic with URL restoration and duplicate handling |
| adminpages/sidebars.php | Updates delete button styling for consistency |
| adminpages/menus.php | New file implementing menu creation, duplication, deletion, and management interface |
| adminpages/admin_header.php | Adds Custom Menus tab to navigation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( ! isset( $_GET['page'] ) || $_GET['page'] !== 'memberlite-custom-menus' ) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The GET parameters 'page', 'action', and 'error' are used without sanitization before being checked. While the comparison with string literals provides some protection, it's a best practice to sanitize input parameters before use. Consider sanitizing these parameters using sanitize_text_field() or sanitize_key() before performing comparisons.
| if ( ! isset( $_GET['page'] ) || $_GET['page'] !== 'memberlite-custom-menus' ) { | |
| return; | |
| } | |
| if ( ! isset( $_GET['page'] ) ) { | |
| return; | |
| } | |
| $page = sanitize_key( wp_unslash( $_GET['page'] ) ); | |
| if ( 'memberlite-custom-menus' !== $page ) { | |
| return; | |
| } |
| $label = isset( $item_data['label'] ) ? sanitize_text_field( $item_data['label'] ) : ''; | ||
| $url = isset( $item_data['url'] ) ? $item_data['url'] : ''; | ||
|
|
||
| // Convert relative URLs to absolute using the current site URL. | ||
| if ( ! empty( $url ) && strpos( $url, '/' ) === 0 && strpos( $url, '//' ) !== 0 ) { | ||
| $url = home_url( $url ); | ||
| } | ||
|
|
||
| $url = esc_url_raw( $url ); | ||
|
|
There was a problem hiding this comment.
The relative URL detection logic checks if a URL starts with '/' and doesn't start with '//' to identify relative URLs. However, this doesn't account for protocol-relative URLs that might be stored in other formats, or URLs that might have been malformed during export. While the check for '//' prevents protocol-relative URLs from being incorrectly converted, consider adding validation to ensure the resulting absolute URL is valid after conversion.
| $label = isset( $item_data['label'] ) ? sanitize_text_field( $item_data['label'] ) : ''; | |
| $url = isset( $item_data['url'] ) ? $item_data['url'] : ''; | |
| // Convert relative URLs to absolute using the current site URL. | |
| if ( ! empty( $url ) && strpos( $url, '/' ) === 0 && strpos( $url, '//' ) !== 0 ) { | |
| $url = home_url( $url ); | |
| } | |
| $url = esc_url_raw( $url ); | |
| $label = isset( $item_data['label'] ) ? sanitize_text_field( $item_data['label'] ) : ''; | |
| $url = isset( $item_data['url'] ) ? $item_data['url'] : ''; | |
| $converted_relative = false; | |
| // Convert relative URLs to absolute using the current site URL. | |
| if ( ! empty( $url ) && strpos( $url, '/' ) === 0 && strpos( $url, '//' ) !== 0 ) { | |
| $url = home_url( $url ); | |
| $converted_relative = true; | |
| } | |
| $url = esc_url_raw( $url ); | |
| // If we converted a relative URL, ensure the resulting absolute URL is valid. | |
| if ( $converted_relative && ( empty( $url ) || ! wp_http_validate_url( $url ) ) ) { | |
| continue; | |
| } |
| // Insert the new menu item. | ||
| $new_item_id = wp_insert_post( $args ); | ||
|
|
||
| // Store the ID mapping. | ||
| $id_map[ $menu_item->db_id ] = $new_item_id; | ||
|
|
||
| // Update parent relationship using the mapped ID. | ||
| if ( ! empty( $menu_item->menu_item_parent ) && isset( $id_map[ $menu_item->menu_item_parent ] ) ) { | ||
| update_post_meta( $new_item_id, '_menu_item_menu_item_parent', $id_map[ $menu_item->menu_item_parent ] ); | ||
| } |
There was a problem hiding this comment.
The wp_insert_post function can return a WP_Error on failure, but this is not checked before using $new_item_id. If wp_insert_post fails and returns a WP_Error object, it will still be stored in the id_map and used in update_post_meta, which could cause issues. Add a check to verify that $new_item_id is not a WP_Error before adding it to the map and updating meta.
| $duplicate_name = $menu_name; | ||
| $counter = 1; | ||
| while ( wp_get_nav_menu_object( $duplicate_name ) ) { | ||
| $counter++; | ||
| $duplicate_name = $menu_name . ' (' . $counter . ')'; | ||
| } | ||
|
|
There was a problem hiding this comment.
The while loop that generates numbered duplicate menu names (lines 362-365) doesn't have a safety limit. While unlikely, if there are already many menus with numbered names (e.g., "Main Menu (2)", "Main Menu (3)", etc.), this could theoretically create very long menu names or take a long time to find an available name. Consider adding a maximum iteration limit (e.g., 100) and handling the case where a unique name cannot be found within that limit.
| $duplicate_name = $menu_name; | |
| $counter = 1; | |
| while ( wp_get_nav_menu_object( $duplicate_name ) ) { | |
| $counter++; | |
| $duplicate_name = $menu_name . ' (' . $counter . ')'; | |
| } | |
| $duplicate_name = $menu_name; | |
| $counter = 1; | |
| $max_duplicate_iterations = 100; | |
| while ( wp_get_nav_menu_object( $duplicate_name ) && $counter <= $max_duplicate_iterations ) { | |
| $counter++; | |
| $duplicate_name = $menu_name . ' (' . $counter . ')'; | |
| } | |
| // If we couldn't find an available duplicate name within the limit, skip this menu. | |
| if ( wp_get_nav_menu_object( $duplicate_name ) ) { | |
| continue; | |
| } |
| <script> | ||
| function memberliteConfirmMenuDeletion( text, url ) { | ||
| if ( window.confirm( text ) ) { | ||
| window.location = url; | ||
| } | ||
| } |
There was a problem hiding this comment.
The JavaScript function memberliteConfirmMenuDeletion uses window.confirm() which can have accessibility issues for screen reader users and doesn't follow modern UX patterns. Consider implementing a more accessible confirmation dialog using WordPress's built-in modal functionality or ensuring the confirm dialog provides adequate context for assistive technologies.
| <script> | |
| function memberliteConfirmMenuDeletion( text, url ) { | |
| if ( window.confirm( text ) ) { | |
| window.location = url; | |
| } | |
| } | |
| <div id="memberlite-delete-menu-modal" class="memberlite-modal" role="dialog" aria-modal="true" aria-labelledby="memberlite-delete-menu-modal-title" hidden> | |
| <div class="memberlite-modal__backdrop" data-memberlite-modal-close="true"></div> | |
| <div class="memberlite-modal__dialog" role="document"> | |
| <h2 id="memberlite-delete-menu-modal-title"><?php esc_html_e( 'Delete menu', 'memberlite' ); ?></h2> | |
| <p id="memberlite-delete-menu-modal-message"></p> | |
| <div class="memberlite-modal__actions"> | |
| <button type="button" class="button button-secondary" id="memberlite-delete-menu-cancel"> | |
| <?php esc_html_e( 'Cancel', 'memberlite' ); ?> | |
| </button> | |
| <button type="button" class="button button-primary button-danger" id="memberlite-delete-menu-confirm"> | |
| <?php esc_html_e( 'Delete menu', 'memberlite' ); ?> | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| <style> | |
| .memberlite-modal[hidden] { | |
| display: none; | |
| } | |
| .memberlite-modal { | |
| position: fixed; | |
| inset: 0; | |
| z-index: 100000; | |
| } | |
| .memberlite-modal__backdrop { | |
| position: absolute; | |
| inset: 0; | |
| background: rgba(0, 0, 0, 0.5); | |
| } | |
| .memberlite-modal__dialog { | |
| position: relative; | |
| max-width: 480px; | |
| margin: 10vh auto; | |
| padding: 20px; | |
| background: #fff; | |
| box-shadow: 0 2px 10px rgba(0,0,0,.3); | |
| } | |
| .memberlite-modal__actions { | |
| margin-top: 20px; | |
| display: flex; | |
| justify-content: flex-end; | |
| gap: 10px; | |
| } | |
| </style> | |
| <script> | |
| ( function () { | |
| var modal = document.getElementById( 'memberlite-delete-menu-modal' ); | |
| if ( ! modal ) { | |
| return; | |
| } | |
| var messageEl = document.getElementById( 'memberlite-delete-menu-modal-message' ); | |
| var cancelButton = document.getElementById( 'memberlite-delete-menu-cancel' ); | |
| var confirmButton = document.getElementById( 'memberlite-delete-menu-confirm' ); | |
| var previouslyFocusedElement = null; | |
| var targetUrl = null; | |
| function openModal( text, url ) { | |
| targetUrl = url; | |
| if ( messageEl ) { | |
| messageEl.textContent = text; | |
| } | |
| previouslyFocusedElement = document.activeElement; | |
| modal.removeAttribute( 'hidden' ); | |
| if ( cancelButton ) { | |
| cancelButton.focus(); | |
| } | |
| document.addEventListener( 'keydown', handleKeydown, true ); | |
| } | |
| function closeModal() { | |
| if ( modal.hasAttribute( 'hidden' ) ) { | |
| return; | |
| } | |
| modal.setAttribute( 'hidden', 'hidden' ); | |
| targetUrl = null; | |
| document.removeEventListener( 'keydown', handleKeydown, true ); | |
| if ( previouslyFocusedElement && typeof previouslyFocusedElement.focus === 'function' ) { | |
| previouslyFocusedElement.focus(); | |
| } | |
| } | |
| function handleKeydown( event ) { | |
| if ( event.key === 'Escape' || event.key === 'Esc' ) { | |
| event.preventDefault(); | |
| closeModal(); | |
| } | |
| } | |
| if ( cancelButton ) { | |
| cancelButton.addEventListener( 'click', function ( event ) { | |
| event.preventDefault(); | |
| closeModal(); | |
| } ); | |
| } | |
| if ( confirmButton ) { | |
| confirmButton.addEventListener( 'click', function ( event ) { | |
| event.preventDefault(); | |
| if ( targetUrl ) { | |
| window.location = targetUrl; | |
| } | |
| } ); | |
| } | |
| modal.addEventListener( 'click', function ( event ) { | |
| if ( event.target && event.target.getAttribute( 'data-memberlite-modal-close' ) === 'true' ) { | |
| event.preventDefault(); | |
| closeModal(); | |
| } | |
| } ); | |
| // Preserve the existing public API used elsewhere in this page. | |
| window.memberliteConfirmMenuDeletion = function ( text, url ) { | |
| openModal( text, url ); | |
| }; | |
| } )(); |
| menuList.querySelectorAll('.memberlite_clickable').forEach(function(row) { | ||
| row.addEventListener('click', function(e) { | ||
| if (e.target.tagName !== 'INPUT') { | ||
| var checkbox = row.querySelector('input[type="checkbox"]'); | ||
| checkbox.checked = !checkbox.checked; | ||
| checkbox.dispatchEvent(new Event('change')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
When clicking on a label element within the clickable row, both the label's default behavior (which toggles its associated checkbox) and the row's click handler will fire, causing the checkbox to toggle twice and effectively remain in its original state. The condition on line 109 only excludes INPUT elements, not LABEL elements. Update the condition to also exclude labels: if (e.target.tagName !== 'INPUT' && e.target.tagName !== 'LABEL')
| } else { | ||
| // Non-replace mode: Create duplicate menu, but assign original to locations. | ||
| $location_menu_id = $existing_menu->term_id; | ||
|
|
||
| // Create duplicate with numbered name. | ||
| $duplicate_name = $menu_name; | ||
| $counter = 1; | ||
| while ( wp_get_nav_menu_object( $duplicate_name ) ) { | ||
| $counter++; | ||
| $duplicate_name = $menu_name . ' (' . $counter . ')'; | ||
| } | ||
|
|
||
| $menu_id = wp_create_nav_menu( $duplicate_name ); | ||
|
|
||
| if ( is_wp_error( $menu_id ) ) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
In the non-replace mode when a menu with the same name exists, the location assignment is set to use the existing menu's ID (line 357), not the newly created duplicate menu's ID. This means that when importing menus without replacing, the menu location assignments will point to the existing menu rather than the newly imported duplicate. This is likely intentional based on the comment "assign original to locations", but this behavior may be confusing to users who expect the imported menu to be assigned to the locations specified in the export file. Consider documenting this behavior more clearly in the UI or adjusting the logic if this is not the intended behavior.
| // Export theme mods if selected. | ||
| if ( ! empty( $_POST['memberlite_export_theme_mods'] ) ) { | ||
| // All theme mods = all Customizer settings for the active theme. | ||
| $mods = get_theme_mods(); | ||
| if ( ! is_array( $mods ) ) { | ||
| $mods = array(); | ||
| } | ||
| $data['mods'] = $mods; | ||
|
|
||
| /** | ||
| * Filter the option keys to export when exporting Memberlite theme settings. | ||
| * By default, we export the site icon, custom sidebars, and sidebar assignments for custom post types. | ||
| * | ||
| * Note: This same filter is used for resetting options in memberlite_reset_theme_settings(). | ||
| * | ||
| * @since 6.1 | ||
| * @param array $option_keys Array of option keys to export. | ||
| */ | ||
| $option_keys = apply_filters( | ||
| 'memberlite_export_option_keys', | ||
| array( | ||
| 'site_icon', | ||
| 'memberlite_cpt_sidebars', | ||
| 'memberlite_custom_sidebars', | ||
| ) | ||
| ); | ||
|
|
||
| $options = array(); | ||
|
|
||
| foreach ( $option_keys as $key ) { | ||
| $value = get_option( $key, null ); | ||
| if ( null !== $value ) { | ||
| $options[ $key ] = $value; | ||
| } | ||
| } | ||
|
|
||
| foreach ( $option_keys as $key ) { | ||
| $value = get_option( $key, null ); | ||
| if ( null !== $value ) { | ||
| $options[ $key ] = $value; | ||
| $data['options'] = $options; | ||
|
|
||
| if ( function_exists( 'wp_get_custom_css' ) ) { | ||
| $data['wp_css'] = wp_get_custom_css(); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the user unchecks the "Theme Settings" checkbox and doesn't select "Include Navigation Menus", the export will create a JSON file containing only the template name without any actual settings or menus. While technically valid, this could be confusing to users. Consider either: 1) making "Theme Settings" disabled/unchecked by default to prevent accidental empty exports, 2) adding a check to prevent exporting if both options are unchecked, or 3) adding client-side validation to warn users before creating an essentially empty export.
| } elseif ( isset( $_GET['error'] ) ) { | ||
| switch ( $_GET['error'] ) { | ||
| case 'duplicate': | ||
| $message = __( 'There was an error duplicating the menu.', 'memberlite' ); | ||
| break; | ||
| case 'delete': | ||
| $message = __( 'There was an error deleting the menu.', 'memberlite' ); | ||
| break; | ||
| case 'empty_name': | ||
| $message = __( 'Please enter a valid menu name.', 'memberlite' ); | ||
| break; | ||
| case 'name_exists': | ||
| $message = __( 'A menu with that name already exists. Please choose another name.', 'memberlite' ); | ||
| break; | ||
| case 'create': | ||
| $message = __( 'There was an error creating the menu.', 'memberlite' ); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The GET parameter 'error' is used in a switch statement without sanitization. While the switch-case structure provides some validation, it's a best practice to sanitize the value using sanitize_key() before use to ensure data integrity.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
All Submissions:
Changes proposed in this Pull Request:
This PR adds a new Custom Menus management page to the Memberlite admin area and enhances the theme import/export tools to support navigation menus.
New Features:
1. Custom Menus Admin Page (Memberlite > Custom Menus)
2. Menu Export Support
3. Menu Import Support
4. Admin UI Improvements
How to test the changes in this Pull Request:
Testing Custom Menus Page:
Testing Menu Export:
menusarray with menu data./about/instead ofhttps://example.com/about/).Testing Menu Import:
Testing Cross-Environment Portability:
Other information:
Changelog entry