New fields added to the menu cannot be exported

Thanks for the patch.

What was the idea of implementing some hooks for this instead of automatically export/import all fields ? Because this would require an extra task to create a custom module in order to fully export/import a menu with fields.

The method suggested in structure_sync.api.php wasn't working for us, probably because we're on an old Drupal (8.9.20) as this code was returning an empty value $options = $menuLink->link->first()->options;

So here is an implementation using the API to find the fields of the menu and export them. It will however work only for single valued fields and would need to be extended to entity reference, formatted text etc...

/**
 * Implments hook_structure_sync_menu_link_export_alter() to deploy field_subtitle.
 */
function souk_deploy_structure_sync_menu_link_export_alter(&$item, \Drupal\menu_link_content\Entity\MenuLinkContent $menuLink) {
  // Retrieve all extra fields from the menu link and export them
  $fields = $menuLink->getFieldDefinitions();
  foreach ($fields as $field_name => $field_definition) {
    if (!empty($field_definition->getTargetBundle())) {
      if (!$menuLink->{$field_name}->isEmpty()) {
        $item[$field_name] = $menuLink->{$field_name}->value;
      }
    }
  }
}

FYI, this issue ✨ Support for translations of menu items Needs review contains code that handles extra fields besides multilingual.

Hi everyone,

Thank you very much for raising this issue and for contributing a patch, it's greatly appreciated.

It actually helped us a lot with a very standard/simple use case:

Exporting config added by the Menu Link Attributes → module.
The module allows adding additional attributes to menu link items, such as 'class', 'id', 'rel', etc... and any other custom attributes, saved in the DB table 'menu_link_content_data', in the serialized field 'link__options'.

In its current state, the Structure Sync module doesn't seem to be able to export the data contained in 'link__options', see:
https://git.drupalcode.org/project/structure_sync/-/blob/51a15a1ba9ecaf7...
(as explained in the issue summary)

Thus requiring additional actions or code in a project to deploy these content changes from one environment to another (for example, manually, update hooks, etc...).

As suggested at #10:

What was the idea of implementing some hooks for this instead of automatically export/import all fields ? Because this would require an extra task to create a custom module in order to fully export/import a menu with fields.

We could indeed try to think of a "one-fits-all" solution which would take into account many different cases and export automatically everything to the structure_sync.data config... But trying to cover all the possible cases and options seems like a rather big step...
For example, if we'd wanted to only cover this particular use case, we could simply add 'link__options' to the array of elements from the link above, the same way it is currently being done in related ticket ✨ Support link options on menu item RTBC (with patch).
However, that would only cover a single very specific use case... See for example ✨ Support Layout Builder on menu item Active , where other users would like to export Layout Builder configuration (key: layout_builder__layout, with the exact same changes as for the link options patch, just above.

Therefore, I personnally think the approach suggested in this patch provides a very flexible solution and possibility to allow anyone to export/import additional data to structure_sync.data, whether from contrib, a completely custom module or set of requirements.
Additionally, it is also inobtrusive in the fact it doesn't have any impact on module's current logic.
Since the import/export of configuration is more of a backend/deployment operation, performance or other considerations shouldn't necessarily be high on the list of concerns, as opposed to flexibility.

Hoping we could get more feedback and reviews, I went ahead and made a few additional "clean-up" changes and documentation improvements at #12.
One thought though, the two import hooks could potentially be grouped together, with two operations: 'create' and 'update' (defaults to 'create'), based on the feedback and reviews received on this patch.
 

Please find below the code we used along with this patch to support the data added by the Menu Link Attributes → module:
Custom module file: contrib10_structure_sync.module

<?php

/**
 * @file
 * Provides 'structure_sync' hook implementations to export additional fields.
 *
 * By default, the Structure Sync module doesn't allow exporting additional
 * fields, in this particular case, for the Menu Link Attributes module, which
 * adds 'class', 'rel', 'target', etc...
 *
 * Based on the Structure Sync API added in patch from issue on Drupal.org:
 * Without the patch, none of these hooks work.
 *
 * @see: https://www.drupal.org/project/structure_sync/issues/2980893
 */

use Drupal\menu_link_content\Entity\MenuLinkContent;

/**
 * Implements hook_structure_sync_menu_link_export_alter().
 */
function contrib10_structure_sync_structure_sync_menu_link_export_alter(&$item, MenuLinkContent $menu_link) {
  // Export the values of the fields added by the 'menu_link_attributes' module.
  if (!empty($menu_link->link->options['attributes'])) {
    $item['menu_link_attributes'] = $menu_link->link->options['attributes'];
  }
}

/**
 * Implements hook_structure_sync_menu_link_import_alter().
 */
function contrib10_structure_sync_structure_sync_menu_link_import_alter(&$menu_item, $config) {
  // Import the 'menu_link_attributes' fields from configuration, if the menu
  // item does not exist: Import safe and force ((re)creates the item).
  if (!empty($config['menu_link_attributes'])) {
    $menu_item['link']['options']['attributes'] = $config['menu_link_attributes'];
  }
}

/**
 * Implements hook_structure_sync_menu_link_update_alter().
 */
function contrib10_structure_sync_structure_sync_menu_link_update_alter(MenuLinkContent &$menu_item, $config) {
  // Import the 'menu_link_attributes' fields from configuration, if the menu
  // item already exists: Import full (with update: merge values from config).
  if (!empty($config['menu_link_attributes'])) {
    $menu_item->link->options += [
      'attributes' => $config['menu_link_attributes'],
    ];
  }
}

Although this ticket and patch only cover Menu Link content items, I certainly think it is a step in the right direction and could perhaps be further abstracted/extended to cover all objects currently supported by the module, in particular, Block Content items (see for example, #3130854: Layout Builder Blocks Export → ) and Taxonomy Terms.

Maybe some middle ground could be found, where patches could be accepted for specific keys (for example, from Drupal Core) to be included by default in the module (such as the 'options' key from patch #3073810 ✨ Support link options on menu item RTBC ), and still have this patch/feature to allow extra/custom config to be added by contrib or custom modules.

In any case, we would greatly appreciate if you could please try testing the patch, give us your reviews and opinions on the approach taken in this ticket.

Feel free to let us know at any point if you have any questions or concerns on any aspects of this patch or the ticket in general, we would surely be very happy to help.
Thanks again very much to everyone who contributed to coming up with the initial idea and code implementation.

Adding another very relevant ticket ✨ Move import/export logic into plugin Active with an approach through plugins which could probably be a preferable way to go, instead of API hooks, but no implementation has been suggested yet.
If anybody has some time, pointers, guidelines or suggestions on a possible implementation with plugins would be greatly appreciated.

Thanks in advance!

This issue is about tweaking the fields to be exported/imported
https://www.drupal.org/project/structure_sync/issues/3085904 ✨ Move import/export logic into plugin Active is about rewriting the whole module using drupal's plugin system as a base to be able to support other entity types.

For this issue, I would agree it would be nicer to use drupal's Plugin system instead of hooks
But the approach here feels ok with hooks at least as a first step

From my point of view, the export/import all available fields approach would not fit this module, it should probably remain light and precise

If others share this point of view (open for debate), I would suggest these next steps:
- Add tests
- Review and test manually
- Add something about this feature in the readme.md
- Opening other (related) issues to cover blocks and taxonomy terms
- (optionnaly, converting the hooks to plugin managers)

To make this easier to review, I've created a merge request.

A brief glance at the code in the merge request suggests that it has no tests. The Structure Sync maintainers prefer to accept merge requests that have passing automated tests. If you need help writing tests, please ask: I would be happy to help!

mparker17 → changed the visibility of the branch 2.x to hidden.

Thanks @mparker17!

RE: #17

The Structure Sync maintainers prefer to accept merge requests that have passing automated tests. If you need help writing tests, please ask: I would be happy to help!

Is this something you could help adding to the module?

We could perhaps:

Add a small Test sub-module that would be enabled in a Test class which could trigger implementations of the hooks introduced in the merge request:
https://git.drupalcode.org/project/structure_sync/-/merge_requests/20/di...

• hook_structure_sync_menu_link_export_alter
• hook_structure_sync_menu_link_import_alter
• hook_structure_sync_menu_link_update_alter

 
Extend or build on top of existing FunctionalJavascript Test class: FunctionalJavascript/StructureSyncMenuLinksTest.php
https://git.drupalcode.org/project/structure_sync/-/blob/2.x/tests/src/F...

So all these hooks could be triggered by the operations tested in this class.
 

Note: I've noticed there are PHPCS errors currently in the MR, unrelated to the changes from this ticket.

We would certainly be glad to hear your opinion on the suggested Testing approach.
Otherwise, any help testing, feedback, reviews, questions or comments would be greatly appreciated.
Thanks in advance!

@dydave, I think that the approach you proposed in #20 (i.e.: adding a test module and extending FunctionalJavascript/StructureSyncMenuLinksTest.php) will be good!

Do you feel comfortable writing the first draft of the test? If not, I can try, and pass it to you for refinement!

As discussed in ✨ Drop support for D8, D9, D10.0-10.3 Active , you don't have to worry about the SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue.NullabilityTypeMissing PHPCS lints; only new lints.

Thanks a lot @mparker17 for all the nice and prompt replies, it's greatly appreciated! 🙏

Do you feel comfortable writing the first draft of the test? If not, I can try, and pass it to you for refinement!

I'm very sorry, but frankly, I don't think I would have the time right now:
I'm working on improving the Admin Toolbar → module these days and there's quite a lot to do actually 😅
In particular updating some of the JS libraries.

I've got plenty of tickets, MRs and support requests to answer already 👌

Not to mention several D11 compatible tickets I'm still trying to push 😅

But I would be happy to review or test any code produced 👍

Thanks again very much for keeping this module well maintained!