Settings no longer stored as third party settings

Thanks for reporting!

I guess really not a regression as that was a dev version.
I guess that was not really intended either.
We could fix it, but then it would need an update as we now made a stable release.

Hello! We can't use third-party settings for Menu Multilingual, because Context module doesn't support them (related issue https://www.drupal.org/project/menu_multilingual/issues/2917338).
I've created the Support request with the details https://www.drupal.org/project/context/issues/2992000

We need to wait fix this until context module has third party settings support.

The module in still in alpha, so an upgrade path is not required. But without one, this change will break all existing sites that use this module.

Added an upgrade path and test coverage.

Thanks a lot for the effort.

The problem is that this patch breaks the integration with Context module.
The tests are passing because we are still missing tests for context integration #3037173: Create tests
I tested it manually, but it doesn't seem to work. I don't get the Menu Multilingual options inside menu blocks within contexts.

Does this patch still break context integration now that #2953766: Provide a Configuration Schema is committed?

@JeroenT
I tested the patch again now using Context version 8.x-4.0-beta5 where #2953766: Provide a Configuration Schema should be included, however the menu multilingual config options are not displayed on menu blocks inside the contect UI.

I think this issue needs to be solved first #2992000: Third party settings support

Back to needs review now that #2992000: Third party settings support is committed.

This issue needs to be tested with the dev version of context.

Great, I have already tested and it seems to work now!
Will just look more carefully over the patch.
I also think we should wait until there is a new release for context module which include the patch available.

Ok, the context module now has a stable version with included patch (8.x-4.1 5 August 2021)
So, we need to fix our test then here #3161849: Tests fail.

Patch no longer applied, so I created a reroll.
Context integration seems to be broken so I added test coverage for context here. Since this patch should fix that.

Issue is still postponed because of #3228412: Replace deprecated code in tests

@JeroenT, I've enabled "Run on commit and for issues" option.

Thanks @vlad.dancer, seems to be working. See #3228412: Replace deprecated code in tests

Ok, now we've updated deprecated code in tests.

According to me, the patch is almost ready.

The context tests are failing with a strict config schema check, but this needs changes in context. Created an issue for that #3228540: Provide a config schema for block reaction - third party settings

And I created as a follow-up once the context issue is fixed: #3228543: Run context tests with a strict config schema check..

I reverted the changes in MenuMultilingualLinkTreeModifier.php, so a rebase should be straightforward for both issues.

I rebased this merge request.

@JeroenT what's the status here? Is the MR final?

The changes look very good and important. Are you using this in production already?
It also adds very important tests, so let's please finalize this :)

I'll tag this for review. Any active maintainer who can also have a look? Otherwise, would you perhaps jump in as Co-Maintainer @JeroenT?

As of the general importance

Tests fail, update require-dev to newest context version.

Hey Mats, just tested the patch and found no major issues, only a couple of notes.

I've tried to test path on Gitpod env via Drupalpod.
And it seems Drupalpod do not install require-dev packages when you create env right from browser extension.

Automatic tests.
In order to run tests there I run manually `composer require drupal/context:^5.1` and similar for menu_block module.

1. minor issue is in composer deps.

+++ b/composer.json
@@ -19,6 +19,7 @@
+    "drupal/context": "^5.1"

There is no 5.1 version for context module but composer will install dev version instead:

composer show -a drupal/context
...
versions : * 5.x-dev, 5.0.0-rc1, 4.x-dev, 4.1.0, 4.0.0, 4.0.0-beta6, 4.0.0-beta5, 4.0.0-beta4, 4.0.0-beta3, 4.0.0-beta2, 4.0.0-beta1, 1.x-dev, 1.0.0-alpha1, 0.x-dev, dev-1.x, dev-0.x, * dev-5.x, dev-4.x

I'm not sure but we can update it to ^5.0 or even specify beta. BTW: I used context v4.1 for manuall testing, so this is really minor.

Manuall testing.
I used system menus with blocks + context module.
Created a couple of menu links and translated them, show them in blocks. No issues, works great, as expected.
Then I ensured that third party settings are in place within exported configs:

context.context.test_context.yml
-----
name: test_context
reactions:
  blocks:
    id: blocks
    blocks:
      00d9886c-364f-44c6-85f4-a4d86d2adb9e:
        id: 'system_menu_block:main'
        label: 'Main navigation'
        provider: system
        custom_id: system_menu_block_main
        ...
        third_party_settings:
          menu_multilingual:
            only_translated_labels: 1
            only_translated_content: false
====
block.block.olivero_main_menu.yml
----
third_party_settings:
  menu_multilingual:
    only_translated_labels: true
    only_translated_content: true
...
id: olivero_main_menu
plugin: 'system_menu_block:main'
settings:
  id: 'system_menu_block:main'
  ...
  provider: system
  level: 1
  depth: 2
  expand_all_items: true

Now, small code nitpeaks:)
PHPCS didn't provided any helpfull advices.
But

+++ b/tests/fixtures/update/menu-multilingual-block-settings.php
@@ -0,0 +1,58 @@
+ * Contains database additions to search-api-db-base.php.
+ *
+ * Can be used for setting up a base Search API sorts installation.

Seems like odd strings related to search api. Let's update them.

That's all!

Greak work @matsbla, @Jeroen!
Thanks @Anybody for pushing forward.

@matsbla please commit your changes to this issue's issue fork, or at least provide an interdiff with your changes. Especially since all the work was already committed on this branch! This makes rebasing the branch, working on the branch and general visibility way easier for others!

Well, I updated fork and now @Grevil your turn with help :)
You could help us in the next ways:
- create interdiff and ensure that changes were done only in composer.json,
- or review and give feedback on patch/fork
- or do manual testing
- or run automated tests locally

@vlad.dancer, thanks!

On it! :)

We should think about either dropping the Drupal 8 compatibility entirely, or instead create a new 2.x branch where we drop the Drupal 8 compatibility and drop the Drupal 10 compatibility in the "old" branch. The reasoning behind this is, that "drupal/context" 4.1 is not Drupal 10 compatible, making this module not compatible either. Your decision @vlad.dancer and @matsbla!

I also found this issue here: #3348699: Enable strict config schema for tests, simply removing the variable will lead to the tests failing, which would be out of scope of this issue, so I created a seperate issue.

Wow, @Grevil. That's great feedback! Thank you.

Found another tiny issue, otherwise code looks great and tests run successful locally! Great work! :)

Let's discuss #38 and then get this stuff committed soon! If we decide to use drupal/context:5.0-rc and drop the D8 compatibility, we should quickly do some further manual testing.

@vlad.dancer, glad I could help! :)

Great work @Grevil! Let's set this RTBC for maintainer review and decision regarding #38. I'd vote for a new 2.x semver branch.

Thank you folks! Just merged and commited.
I created 2.x branch, let's plan integration with drupal/context:5.0 now.