🇨🇦Canada @mparker17

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

@dydave, thank you!

1. I'm open to enabling OPT_IN_TEST_MAX_PHP if you think that it is necessary.
   1. In the past, I've run into more issues with new code not being compatible with old versions of PHP than issues with new versions of PHP not being compatible with existing code. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.
2. I'm open to enabling OPT_IN_TEST_PREVIOUS_MINOR, and OPT_IN_TEST_NEXT_MINOR if you think they are necessary.
   1. Generally, I've found that testing the major versions is good enough, because trying to maintain backwards compatibility typically prevents me from being able to use new APIs for a while anyway. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.

That being said, I am respectfully pushing back on a few of the changes in the merge request...

1. OPT_IN_TEST_NEXT_MAJOR will always fail, because that currently refers to Drupal 12, and structure_sync doesn't support Drupal 12 yet.
   1. We could change the core_version_requirement to start supporting D12, in order to make those tests pass - but until Drupal 12 reaches alpha, D12 tests will start/stop failing randomly as D12 changes. I've found this discourages new contributors who don't understand that...
      1. the failing D12 test is not their fault, and,
      2. they don't have to fix D12 failures until D12 reaches alpha.

      ... I've found contributors will sometimes abandon an issue if they don't understand how to fix a test fail.

   2. My best practice is to only start caring about compatibility with the next major version of Drupal core when it reaches alpha, and to work on compatibility in an issue branch, so it doesn't affect the mainline until all the bugs have been worked out.
2. Based on experience with other projects, _PHPUNIT_CONCURRENT (or rather, the thing that variable controls, paratest ✨ Run phpunit tests from a single job in parallel Fixed ) often results in random test fails, especially when running Functional and FunctionalJavascript tests, due to the nature of those tests.
   1. As mentioned in the previous point, new contributors sometimes misunderstand random test fails as something that they did incorrectly, which discourages them from contributing (or makes them abandon the issue).
3. I generally don't require all jobs to pass, because - as a maintainer - I haven't found that to be necessary:
   1. Most contributors will already try to make all the tests pass, i.e.: without me asking.
   2. Any contributors who don't will usually fix it if I ask.
   3. If I really feel strongly that an issue needs to get merged right away, I will fix the code on my own.
   4. Requiring all jobs to pass in .gitlab-ci.yml will also enforce that on the mainline branches, and on security team branches (i.e.: and prevent me from merging to those branches if those lints fail). If I was to be contacted by the security team → , I have a duty to the Drupal community to get a fix out as soon as possible. If necessary, I want the option to be able to fix security issues right away, and worry about phpcs, eslint, phpstan, cspell, etc. in follow-up issue(s).

In summary, if you can change the merge request so that it only sets OPT_IN_TEST_MAX_PHP, OPT_IN_TEST_PREVIOUS_MINOR, and OPT_IN_TEST_NEXT_MINOR, I'll merge it for sure.

I'm currently hesitant about merging OPT_IN_TEST_NEXT_MAJOR, _PHPUNIT_CONCURRENT, and requiring the linters to pass; but I'm open to new ideas: if you have compelling reasons why I should use those options, I'd be interested to hear them!

To make this easier to review, I've rebased the merge request onto the latest 2.x.

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!

@dydave, thank you very much, but I cannot apply these changes without dropping support for Drupal 8 (the ?type syntax was introduced in PHP 7.1; but Drupal 8 requires PHP 7.0). Dropping support for D8 would be a backwards-compatibility break.

I've filed a ticket to discuss this, ✨ Drop support for D8, D9, D10.0-10.3 Active - please weigh in there :)

I'm marking this ticket as a duplicate of that one.

mparker17 → changed the visibility of the branch 3074288-url-alias-of to hidden.

I recently found out about the Config Auto Export → module. Could you confirm if it does (or not?)?

If that module doesn't do what you want out-of-the-box (I suspect it might not), we may still need to add something to ping it when one of the things we are tracking changes.

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch? Is it a duplicate of ✨ Support Layout Builder on menu item Active or 🐛 Export conflict with layout builder Active ?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

Is this still an issue in the 2.x branch?

This is an issue for an unsupported branch (8.x-1.x).

Is this still an issue in the 2.x branch?

Marking as "Maintainer Needs More Info", awaiting a response to the question in #16.

There doesn't appear to be any code in the issue fork.

Strange, I can't see any commits in the branch.

mparker17 → made their first commit to this issue’s fork.

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

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

mparker17 → made their first commit to this issue’s fork.

mparker17 → made their first commit to this issue’s fork.

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

mparker17 → made their first commit to this issue’s fork.

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

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 → made their first commit to this issue’s fork.

To make this easier to review, I've created a merge request. The patch didn't apply properly, so I tried to re-create it, but I may have made a mistake in doing so: please ensure that the changes in the merge request fixes the problem and report back here.

Note that this is a patch to an unsupported branch (8.x-1.x). I haven't yet had a chance to determine if this issue is still present in the 2.x branch, nor if this patch would also apply there; @elkaro, do you know if it is still an issue in the 2.x branch?

mparker17 → made their first commit to this issue’s fork.

I'm going to ignore the patch in #12: it changes the README.md back to a README.txt format, which isn't helpful; and thus I am not going to grant credit for #12. Please refer the documentation on Abuse of the Contribution Credit System for more information → .

I have created a merge request from the patch in #6.

I accidentally pushed the commit to 2.x HEAD in commit 2caf7e0 then reverted it in be23c4b - sorry for the noise.

Updated the title to reflect that this has changed from a support request to a feature request.

To make this easier to review, I've rebased the merge request onto the latest 2.x.

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!

I've marked this issue as "Needs title update" and "Needs issue summary update" because it's unclear to me what feature you are trying to add to the module nor what feature is being fixed. I took a look at the code too, but the intent behind the change is still unclear to me.

mparker17 → made their first commit to this issue’s fork.

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 → made their first commit to this issue’s fork.

To make this easier to review, I've created a merge request. The patch also didn't apply for me, so I tried re-creating it: please check that the merge request still solves the problem, and report back here.

My editor automatically corrected some whitespace errors from the patch, but it may have missed some, and I'm confident that there will be other issues (e.g.: the name of the private function, __add_taxonomy_ancestors doesn't follow function naming conventions → ). Look for the new issues reported by PHPCS (and/or PHPStan) in the merge request pipeline.

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 → made their first commit to this issue’s fork.

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 → made their first commit to this issue’s fork.

To make this easier to review, I've rebased the merge request onto the latest 2.x and resolved merge conflicts. While I did my best to resolve the merge conflicts properly, it is possible that I made a mistake: please check to make sure the updated merge request still fixes the issue!

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 → made their first commit to this issue’s fork.

To make this easier to review, I've rebased the merge request onto the latest 2.x.

mparker17 → made their first commit to this issue’s fork.

To make this easier to review, I've rebased the merge request onto the latest commit to the 2.x branch.

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 → made their first commit to this issue’s fork.

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 → made their first commit to this issue’s fork.

Accidentally committed this to 8.x-1.x in commit bad9be3 and had to revert it in 2064289.

I've created the merge request correctly this time; sorry for the noise.

Regardless, what I wrote in #7 still applies.

To make it easier to review, I've created a merge request from the patch in #3.

Note that this is a patch to an unsupported branch (8.x-1.x). I haven't yet had a chance to determine if this issue is still present in the 2.x branch, nor if this patch would also apply there.

mparker17 → made their first commit to this issue’s fork.

A couple of other thoughts...

1. I'm proposing that the 3.0.0 release be - for the most part - the latest 2.0.x release but without support for old versions of Drupal core, I anticipate the move from 2.x to 3.0.x will be pretty trivial for most users.
   • To make things easier, I don't think we should create the 3.0.x branch until we have buy-in from community members and maintainers and we've decided how long to support the 2.x branch
2. Given that I'm anticipating the move from 2.x to 3.0.x will be pretty trivial, I think it's fair for the maintainers to offer security and bugfix support for the 2.x branch for 6 months before marking that branch as unsupported.
   • If we offer to backport bugfixes to 2.x for 6 months, I don't think we need to rush to get bugfixes in before we create the 3.0.x branch
3. Given that we cannot run tests against older versions of Drupal core, and it is non-trivial to manually test on old versions of Drupal core (and I can't remember the last time I manually tested on old versions of Drupal core for this module), I see officially dropping support for old versions of Drupals as the maintainers owning up to the current reality, rather than a big change.
4. As a maintainer, I don't see any more-detailed stats than the rest of the community: it's entirely possible nobody is using this module on old versions of Drupal core, I can't tell. Usage stats for structure_sync → . Usage stats for Drupal core → .

mparker17 → created an issue.

Merging and granting credit. Skipping RTBC because maintainer wrote the patch.

Pipelines pass. Skipping NR step due to the maintainer writing the patch.

Created a merge request.

Merging.

mparker17 → created an issue.

There hasn't been any further feedback from any other maintainers in ~6 months, so I've created a merge request.

Hi @dieterholvoet, thank you for offering to maintain the module!

(I haven't had as much time to maintain the module as I would like recently due to life circumstances; also, the client I'm currently working for doesn't use this module, so I can only contribute as a volunteer)

I have taken a look at some of your recent contributions. I am happy to see that you have experience co-maintaining and contributing to many other modules. Of the contributions that I saw, your code was clear and made good use of APIs, and your interactions with other community members was friendly and thoughtful. I am happy to see that you've written tests, especially in your work on the ECK module → . For all of these reasons, I support your request to co-maintain structure_sync!

I would like to hear from at least 1 other maintainer if possible, so I'm going to mark this as RTBC. I work with @sensespidey (another co-maintainer) so I'll ping him in our company instant messenger to review this issue as well. If we don't hear back from any other maintainers by April 18th, then I will grant you maintainership myself.

For me, one of the most important gates to accepting patches for any module that I maintain is for the patch to have tests, because tests make my job as a maintainer SIGNIFICANTLY easier. When responding to a merge request, I have been trying to remember to offer the author help with writing tests; and for high-priority issues, I try to add tests to the merge request myself if they are missing. Thus, my personal request to you as a new co-maintainer, is to try to do the same. Thank you in advance!

@zengenuity, I've tested this both in mailpit on a dev site; and in real mail clients on a real site, and it works perfectly!

Looking at the code changes, they're straightforward and easy to understand.

As an aside, I'm not sure that I fully understand how style.css and style-tag-styles.css interact with each-other - more documentation would be ideal, but that is out-of-scope for this issue.

Checking the color contrast ratios...

1. #222222 background (i.e.: .email-body in dark mode) and #ffffff foreground (i.e.: regular text in dark mode) have a color contrast ratio of 15.9:1, which passes the WCAG AA and AAA contrast ratios for normal and large text, which is excellent.
2. #222222 background (i.e.: .email-body in dark mode) and rgb(27, 154, 228) = #1B9AE4 foreground (i.e.: link text in dark mode) have a color contrast ratio of 5.14:1, which passes the AA contrast ratio for normal and large text; and passes the AAA contrast ratio for large text, which is great.
   • While testing, I noticed that the color contrast for link text on the light-mode background (#ffffff) only passes the AA contrast ratio for large text, which is a problem, but that is out-of-scope for this issue.
3. rgb(27, 154, 228) = #1B9AE4 background (i.e.: .button in dark mode) and #000000 foreground (i.e.: button text in dark mode) have a color contrast ratio of 6.78:1, which passes the AA contrast ratio for normal and large text; and passes the AAA contrast ratio for large text, which is great.
   • While testing, I noticed that the color contrast for buttons in light mode only passes the AA contrast ratio for large text, which is a problem, but that is out-of-scope for this issue.
4. rgb(20, 117, 173) = #1475ad background (i.e.: .button:hover in dark mode) and #000000 foreground (i.e.: button text in dark mode) have a color contrast ratio of 4.17:1, which only passes the AA contrast ratio for large text, which isn't ideal.
   • But, keeping the light-mode foreground color of #ffffff has a contrast ratio of 5.02:1, which passes the AA contrast ratio for normal and large text; and passes the AAA contrast ratio for large text, which is great.
5. rgb(204, 0, 0) = #cc0000 background (i.e.: .button.danger in dark mode) and #000000 foreground (i.e.: button text in dark mode) have a color contrast ratio of 3.56:1, which only passes the AA contrast ratio for large text, which isn't ideal.
   • But, keeping the light-mode foreground color of #ffffff has a contrast ratio of 5.88:1, which passes the AA contrast ratio for normal and large text; and passes the AAA contrast ratio for large text, which is great.
6. rbg(180, 0, 0) = #b40000 background (i.e.: .button.danger:hover in dark mode) and #000000 foreground (i.e.: button text in dark mode) have a color contrast ratio of 2.94:1, which doesn't pass any WCAG contrast ratio, which isn't ideal.
   • But, keeping the light-mode foreground color of #ffffff has a contrast ratio of 7.14:1, which passes the AA and AAA contrast ratios for normal and large text, which is excellent.

For danger buttons, I think the easiest thing to do to fix color-contrast accessibility issues would be to simply use the light-mode foreground colour of #ffffff... something like...

/* In style-tag-styles.css */
@media (prefers-color-scheme: dark) {
  .button.danger {
    color: #ffffff !important;
  }
}

A solution for regular buttons is a little trickier... I'll have to think about this some more, and I might have some ideas in the morning.

Putting on my maintainer hat, thank you, @nexusnovaz and @tom konda!

SitemapMenuTest::testMenuDepth() looks good to me: thank you very much!

Putting on my developer hat, I think that SitemapUpdateSettingsTest::testUpdateHookN() is failing because the new menu_depth configuration option needs to be added to the configuration schema in config/schema/sitemap.schema.yml — at time-of-writing, there is a schema for Menu plugins between lines 67 and 74 of that file.

I wanted to provide an example of what that configuration might look like, but I could not find an example, because this merge request uses the string unlimited to indicate unlimited menu depth...

1. I searched Drupal core and some contrib modules ( Admin Toolbar → , Better Exposed Filters → , Client-side Hierarchical Select → ) for the search term depth in all *.schema.yml files, to see there were any other examples of "depth" configuration that could either be a string or an integer, but I could not find any examples.
2. It seems like most "menu depth" configuration uses type: integer, where the integer 9 indicates the maximum possible depth.
3. This module's sitemap_plugin_settings.vocabulary schema defines both term_depth and rss_depth as type: integer, but use NULL to indicate unlimited depth (i.e.: by saying both type: integer and nullable: true in the config schema).
4. Other examples of "string or integer" configuration that I could think of (e.g.: system.performance:cache.page.max_age, automated_cron.settings:interval, dblog.settings:row_limit, system.file:temporary_maximum_age, etc.) use a special integer (0, -1) to indicate the string "Unlimited", "Never", "All", etc. That being said, I might have missed something!

In summary, it might be simpler to write a config schema if you changed the merge request to use an integer like -1, or the value NULL to indicate an unlimited depth, instead of what the merge request does now (i.e.: use the string unlimited to indicate an unlimited depth).

For what it's worth, I've found the Configuration Inspector module → to be helpful when writing and debugging configuration schemas.

@darvanen, thanks for responding!

Apologies, I have been quite busy recently — both at-work, and outside-of-work — and thus, I haven't had much free time to contribute to search_api_opensearch or elasticsearch_connector! Thank you for your understanding and patience with me!

That being said, I very much appreciate the community's discussion on the proposed approach — if yourself (or someone watching this issue) has any ideas, suggestions, and/or comments on the feasibility of the proposed approach; or if you can anticipate any pitfalls — I'd love to hear them! Thank you in advance!

@zengenuity, I would be happy to! I should have some time this coming week!

Thank you very much!

mparker17 → created an issue.

Awesome, thank you! I will file a follow-up ticket!

(added some information about how I figured out that the README was wrong to the issue summary)

(Updated the issue summary slightly to be more concise)

Here's a merge request. Reviews welcome!

mparker17 → created an issue.

Awesome, thanks @jurgenhaas! :)

Yay, tests pass now! Reviews welcome!

Here's a patch. Reviews welcome!

mparker17 → created an issue.

I've created a merge request, but still need to add tests.

mparker17 → created an issue.

📌 The whole index gets cleared when any change in the search index configuration is imported Active and 🐛 "Fields to highlight field is required" even when Elasticsearch Highlighter is disabled Active are now done.

Merged! Thanks everyone!

Merged!

Seems to work

@jwilson3, apologies for the delay in replying: thank you very much for the new version!

I haven't had a chance to test the changes in 📌 Refactor Label Help to use hook_field_widget_WIDGET_TYPE_form_alter Active , but in my opinion, there are enough changes to how the module works to warrant a 2.1.x or even 3.0.x branch (i.e.: Route 2 in your comment). (I can post this in 3495740 if you'd like :) ).

Added 🐛 "Fields to highlight field is required" even when Elasticsearch Highlighter is disabled Active and 💬 Issue with Elasticsearch Connector 8.0.0-alpha3 - "No alive nodes" error after upgrade Active as blockers.

Tests are failing, because of 🐛 "Fields to highlight field is required" even when Elasticsearch Highlighter is disabled Active , which is unrelated to this issue. Going to fix that one first, then re-try tests here: if I don't have to modify any operational code, then I'll merge this.

Thanks in advance for your patience!

mparker17 → created an issue.

I resolved a merge conflict in .cspell-project-words.txt, but because cspell is not operational code (it's linter configuration) I feel okay to merge this once tests are green.

Awesome, thanks for the feedback!

Unassigning, now that it is complete.

Moved ✨ Highlighting support (leverage Elasticsearch highlighting) Needs review into the "done" section

Merged! Thanks everyone!

Awesome, thanks @star-szr!

Awesome, tests run now! But they seem to be failing because assertions are failing.

mparker17 → made their first commit to this issue’s fork.

Thanks @alberto56!

I've created a merge request from the patch in #2 ✨ Make 8.x-7.x Drupal 11 compatible Active (the patch didn't apply cleanly in tests/modules/elasticsearch_test/elasticsearch_test.info.yml so I just re-created the change).

I believe tests are failing on the branch so I don't really expect them to pass here, but let's see what Testbot thinks anyway.

mparker17 → made their first commit to this issue’s fork.

Unassigning myself, now that this is Closed (fixed).

Unassigning myself, now that this is Closed (outdated).

Unassigning myself, now that this is Closed (fixed).

Unassigning myself, now that this is Closed (fixed).