New fields added to the menu cannot be exported

I've had the same issue recently, here's a patch adding hooks to the menu sync process.

The 3 hooks are documented in the api file :

- alteration of the exported config : hook_structure_sync_menu_link_export_alter
- alteration of new menu items : hook_structure_sync_menu_link_import_alter
- alteration of menu item being updated : hook_structure_sync_menu_link_update_alter

Each one is filled with the exemple of what I had to do, importing the font awesome icons of menu items. These hooks can be used for anything else, like filling menu attributes.

The seraparation between import and update is not ideal, but I wanted to keep the changes to the module minimal.

New features go into HEAD, which may turn out to be a 2.x branch if we have one.

Any update on exporting menu_item_extras fields with structure_sync module?

I've created an MR that re-rolls patch #2 on `2.x` branch

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 #2960620: Support for translations of menu items 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 #3073810: Support link options on menu item (with patch).
However, that would only cover a single very specific use case... See for example #3367611: Support Layout Builder on menu item, 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), 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 #3085904: Move import/export logic into plugin 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 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!

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 #3517435: Drop support for D8, D9, D10.0-10.3, 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!

Okay, I've created a test stub in the merge request - reviews and refinements welcome. Specifically, did I miss any cases? Is there something else we could test/demonstrate?

Note that, to my surprise, I found the new additions to the menu link data gets duplicated, given the current production code in the merge request and/or the example code added in structure_sync.api.php in this merge request.

menus:
  -
    menu_name: main
    title: Contact
    parent: null
    uri: 'internal:/contact'
    link_title: ''
    description: null
    enabled: true
    expanded: false
    weight: 0
    langcode: en
    uuid: d0429f67-d567-4a4e-a78a-2f1615821326
    options:
      fa_icon: foo
      fa_icon_appearance: bar
    fa_icon: foo
    fa_icon_appearance: bar

... I'm not sure if this was intentional or not, but I dutifully tested this behavior in the test in the merge request.

If this duplication is not intentional, please clarify; and update both the production code and the test code in the merge request.

(assigning credit)

Thanks a lot for getting this over the line @mparker17! 🙏

The tests look great! 👍

I've done an initial read/review of the added MenuLinkExtraConfigTest Test class and more particularly its testExportAndImportWithCustomData Test case and it seems to do everything mentioned above at #20.

✅ The test module with all the new alter hooks structure_sync_hook_test is properly enabled.
✅ Test export_alter: Set a value for a menu link options' key 'structure_sync_hook_test_EXPORTKEY', export it and check the value was exported properly.
✅ Test update_alter: Update the value of the menu link options' key 'structure_sync_hook_test_EXPORTKEY' in the exported config data, import the modified data with the test value and check it was merged properly with the existing menu options.

The tests seem to be passing 🟢 which is great!

Not sure if you wanted to test the import_alter hook though, I "think" this one would need maybe destroying a link, which could be re-imported/re-created with the correct options (?!).
But that would just be an extra nice to have 😅

I hope I didn't misunderstand some of the code and that this review will be useful, since I mostly just read it...

I left another small comment in the MR, but overall it looks very good!
Thanks again very much for the great work! 🙂

Thanks @mparker17 for the prompt and kind reply, once again, it's greatly appreciated.

Sorry I didn't read carefully your previous comment (lack of time and finished work late😅).....

RE: #23

Note that, to my surprise, I found the new additions to the menu link data gets duplicated, given the current production code in the merge request and/or the example code added in structure_sync.api.php in this merge request.

Thanks a lot for raising this point.
🟢 I checked and indeed, this is the intended behavior: The API document demonstrates how various attributes and keys could be modified for a menu link item. In other words, the hooks don't only allow to modify the values in the options array, but also any potential key or attribute that users would like to add to a menu link item.

Do you think this could be misunderstood, unclear or misleading for some users?
These are "just" basic examples without necessarily any specific purpose in mind to illustrate how the parameters could be altered.

Thank you very much for taking this into account in the tests: it wasn't absolutely necessary, but it gives a good example of how the hooks could be implemented with more generic parameters.

@dydave,

🟢 I checked and indeed, this is the intended behavior: The API document demonstrates how various attributes and keys could be modified for a menu link item. In other words, the hooks don't only allow to modify the values in the options array, but also any potential key or attribute that users would like to add to a menu link item.

Awesome, thank you for the clarification! 🙂

Do you think this could be misunderstood, unclear or misleading for some users?

I'm not sure what "this" is referring to in your question.

If you're asking "Do you think the duplicate menu link data in the exported config could be misunderstood, unclear or misleading for some users?", I think it's possible, but maybe this is something we can address in a follow-up issue.