🇨🇦Canada @mparker17

@ajetiv1, for now, the changes are in merge request !4, attached to this issue.

If you'd like a patch (e.g.: for Composer), there are instructions to download a patch in the Using GitLab to contribute to Drupal documentation on Downloading a patch file → .

Thanks everyone for your hard work on this issue!

I'm taking a look at the merge request and trying to understand what it does...

I don't think I've run into a Data Source plugin → before, so please forgive me for the silly question (and for using Elasticsearch-specific terms), but does a Data Source plugin tell Search API how to work with an Index with a dynamic mapping and/or an Index whose content is ingested by third-party crawler (e.g.: Elastic web crawler or Apache Nutch)?

I've added some code in !91; it needs tests; but reviews are welcome.

mparker17 → created an issue.

@alfattal, awesome, I have pushed the commit! Reviews welcome!

@alfattal, I've figured out a way to make the visibility of the "Items selected" details box independent from the visibility of the "Select / Deselect all results (all pages)" checkbox.

I'm uploading it as a patch because it goes a bit further out-of-scope than what @jerrylow originally proposed as their own solution to the issue they filed (also, Hi @jerrylow! 👋). If the other people in this issue agree that this is the direction that they want to go, I'll simply push the commit from my local.

I've uploaded a patch to 4.3.x as is tradition; as well as an interdiff from the most-recent-commit to the merge request (commit 51e5c5c2) to show the proposed changes for someone already familiar with what's in the merge request.

Feedback (from everyone in this issue, as well as the maintainers) is welcome!

You can see in the attached screenshots that I've replaced the "Force show selection info state" with two controls: a control to change the visibility of the "Items selected" details element, and a separate control to change the visibility of the "Select / Deselect all results (all pages)" checkbox.

Screenshot of config page and resulting view, configured to only show the selection box:

Screenshot of config page and resulting view, configured to only show the select all checkbox:

Screenshot of config page and resulting view, configured to show neither selection control

Screenshot of config page and resulting view, configured to show both selection controls:

@alfattal, got it, thanks! I'll take a look!

Yes but from what I remember there may be more conditions. Should check first.

@graber, I apologize, but are you asking me to check if there are more conditions? (I'm happy to try, but I'm asking for clarification because I don't want to be a bottleneck)

Moving back to "Needs Review"

I've pushed an update hook to fix existing views (and I wrote a test for the update hook).

I also put a Choice constraint on the force_selection_info configuration

Looking at the configuration form control, I find it a bit confusing.

    $form['force_selection_info'] = [
      '#type' => 'select',
      '#title' => $this->t('Force show selection info state'),
      '#default_value' => $this->options['force_selection_info'],
      '#description' => $this->t('Should the selection info fieldset be shown above the view even if there is only one page of results and full selection can be seen in the view itself?'),
      '#options' => [
        '' => $this->t('Default Behavior'),
        'show' => $this->t('Always show'),
        'hide' => $this->t('Always hide'),
      ],
    ];

At the very least, I would recommend that we change the key/label of the "Default" option — neither the key (i.e.: an empty string) nor the label ("Default Behaviour") describes what it does very well. Maybe something like 'only_multiple' => $this->t('Only when there are multiple pages'),?

I daresay the title and description could be reworded as well, but I don't want to derail the discussion either.

@graber what are your thoughts as a maintainer?

To prove to myself what was going on, I added a test.

With the patch in this issue...

1. The 'Default Behavior' (i.e.: force_selection_info = '') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results
2. The 'Always show' (i.e.: force_selection_info = 'show') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results
3. The 'Always hide' (i.e.: force_selection_info = 'hide') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results

Given the above behavior, regarding the comment in #27...

The patch in #25 works fine. However, it doesn't provide the option to disable the (Select / deselect all results) which is what this ticket is basically about.

My understanding is that the 'Always hide' (i.e.: force_selection_info = 'hide') is a way to disable the "Select / deselect all results" checkbox? But maybe @alfattal is talking about something else, and I am misunderstanding #27?

We should also add tests.

Interesting, I don't see changes in any patches or merge requests that address the issue identified in #27 📌 Provide a way to disable "select all on all pages" Needs work .

I've turned the patch by @knyshuk.vova in #25 📌 Provide a way to disable "select all on all pages" Needs work into a merge request.

Next, I'll address the issue identified in #27 📌 Provide a way to disable "select all on all pages" Needs work .

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

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

Merging!

Yay, the new test passes!

Updating the issue summary with some more-detailed information to make it easier for people reading the release notes when this actually gets released.

@tom konda, I rebased the branch onto 8.x-2.x. Then I added an update hook (sitemap_update_8205()) that adds a menu_depth to all existing menu plugins. I also updated SitemapUpdateSettingsTest::testUpdateHookN() to test the update hook. Testing out the functionality manually, it seems to work the way I expect, which is excellent. I still need to review the code properly; but overall this is looking good. Thank you!

I've added a tests/src/Functional/DashboardListBuilderTest.php in the latest commits to !59. Reviews welcome!

Wait, it seems like there were some other errors. Re-opening

This looks good to me; merging.

Going to mark ✨ Deprecate the constructors in BlocksController, TaxonomiesController, and MenuLinksController Active as the parent of this issue, because we have to do that one first :)

(moving back to "needs review" to get feedback)

@plopesc wrote,

Removing the "View" local task was made on purpose because having the ability to create or edit a dashboard does not give the access to it.
That's why the "Preview" local task was created, ensuring that users that can create/edit the dashboard can get access to preview, even if they don't have access to it from /admin/dashboard local tasks.

Regarding the penyaskito's approach, that could be an option, but that link would point to a 404 error if the current user does not have access to the dashboard. We might just only make links to those dashboards the current user has access to, but it could be confusing.

Interesting... I hadn't considered that specific scenario.

@penyaskito's buildForm() code alters the table row to add the link... would it be an acceptable compromise to check $entity->toUrl('canonical')->access() before transforming the label into a link, like this...?

public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $dashboardUrl = $entity->toUrl('canonical');
      if ($dashboardUrl->access()) {
        $form[$this->entitiesKey][$entity->id()]['label'] = [
          '#type' => 'link',
          '#title' => $entity->label(),
          '#url' => $dashboardUrl,
        ];
      }
    }
    return $form;
  }

mparker17 → created an issue.

@penyaskito, @plopesc, thank you for the quick replies; I apologize for my own delay in getting back to you!

@penyaskito wrote,

🐛 Wrong redirect after Dashboard layout edition Active would definitely help here. Once you save, you are redirected to the dashboard canonical url instead of the listing.

I agree, this would help, as it would have cleared up some of my own misunderstanding while I was testing.

@penyaskito wrote,

The toolbar caching is definitely an issue, can you create a separate one?

Okay! I will edit this message and post a link here when I've done so.

@penyaskito wrote,

An option could be having on DashboardListBuilder:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $form[$this->entitiesKey][$entity->id()]['label'] = [
        '#type' => 'link',
        '#title' => $entity->label(),
        '#url' => $entity->toUrl('canonical'),
      ];
    }
    return $form;
  }

I like this code better; I'll change my merge request shortly!

I'll reply to @plopesc's message soon.

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

@spiderman, thank you very much!

I'm wondering if it makes sense to get ✨ Deprecate the constructors in BlocksController, TaxonomiesController, and MenuLinksController Active in before cutting a new release, or if you prefer to cut 2.0.9 here, and then the further deprecations in a 2.0.10 release?

I think it's worth it to get #3519479 in before cutting a new release.

Thanks @dydave for the kind words! :)

Thanks @spiderman for filing a follow-up, ✨ Deprecate the constructors in BlocksController, TaxonomiesController, and MenuLinksController Active

Crediting and updating issue summary

I was about to merge this and noticed a new phpcs message...

The deprecation-version 'structure_sync:2.x' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or project:n.n.n-label[n] (Drupal.Semantics.FunctionTriggerError.TriggerErrorVersion)

... so I fixed it in another commit; and verified there were no new errors, so I'm going to merge this.

I still need to add tests for this change.

Updating the title

Discuss if we should add a "View" link to the Primary Tabs that will take the user to view the page.

I'm not sure where this is recorded, but it looks like the "View" link is missing from dashboard.links.task.yml because the page at /admin/dashboard uses the primary links to allow users to switch between the dashboards they are allowed to see — and when you add it, you get tabs for other dashboards and the current dashboard all mixed together, which is confusing, and likely a bad idea.

Maybe a link from /admin/structure/dashboard is sufficient? But I'd be interested to hear feedback from the maintainers, who have put a lot more thought/work into this module than I have.

I've made a patch to link the dashboard label to the canonical URL from entity.dashboard.collection.

Reviews welcome!

In \Drupal\dashboard\DashboardListBuilder, I was forced to change the name of the label column to link in both DashboardListBuilder::buildHeader() and DashboardListBuilder::buildRow(), because I discovered that \Drupal\Core\Entity\DraggableListBuilderTrait forces a column named label (if it exists) to be plain text with the following code in DraggableListBuilderTrait::buildForm():

      if (isset($row['label'])) {
        $row['label'] = ['#plain_text' => $row['label']];
      }

Interesting! After manually flushing caches for the first time, a "Dashboards" link appeared in my Toolbar, so I've deleted that part from the issue summary.

mparker17 → created an issue.

@alberto56, sorry for the delay! I've reviewed this, and it looks good to me! I've merged it: thank you very much!

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

@taiger, thank you!

Your patch in #10 fixes a lot of the clarity and coding-standards issues with the patch in #2, which is great: thank you very much! I hadn't noticed the missing-UUID bug myself, but I will check it out when I incorporate your patches into the merge request when I next have some contribution time.

I also noticed that the filename in the patch you posted to #11 refers to a different issue and unexpected comment number (i.e.: #3177106-4: Support terms with the same name in a tree structure and multi-parent terms → ). The patches seem mostly the same, so I'm going to assume that was unintentional (I couldn't count the number of times that has happened to me).

I worry that we might have been duplicating our efforts this week... it seems that we both spent time this week re-rolling the patch from #2: I committed my re-roll to merge request !41 at the same time that you rewrote #2 and posted your re-write to the patch in #10.

In the future (i.e.: not necessary for this issue), there are some things that you can do to reduce duplicate efforts, and make things easier for maintainers — which will usually result in a faster turn-around time!

As a maintainer, I have to make sure that the patch that I accept fixes the original issue that @kwfinken reported (i.e.: stays within scope → ). I feel reasonably confident that @kwfinken's own patch in #2 fixes the issue (i.e.: because the same person reported the issue and posted the patch). But, because you updated the patch and made your own changes in the patch posted to #10, it is non-trivial for me to see how we got from @kwfinken's patch in #2 to the patch in #10 — that's why it is going to take me some time to incorporate your changes! (I also maintain ~29 other modules with ~1700 issues across all of those modules — and my full-time client doesn't use this module, so I maintain this module on my volunteer time, which is limited by my responsibilities to my work, family, etc.!)

Because lints and tests are automatically run on merge requests → , I find them easier to review than patches these days (and I often convert patches into merge requests for this reason). If there is already a merge request in an issue, please consider contributing to the merge request instead of patches! Here's a video tutorial; or if you prefer written documentation, skim this introduction → ; and read how to clone and commit code to an issue fork → , and how to create a merge request → .

If you prefer to keep working with patches, and the latest patch no longer applies, you can re-roll a patch → . When re-rolling a patch, you can make things easier for a maintainer by making as few changes to the re-roll as you possibly can. Then — once you have posted the re-rolled, minimally-changed patch — make any improvements you want to make in one or more subsequent patches (it also helps to include interdiffs with the subsequent patches → ). By making as few changes as possible in the re-roll, I can easily compare the original patch (i.e.: that didn't apply) to the re-rolled patch: if they are mostly the same, then it's easier for me to see that they both fix the same problem. By making improvements in follow-up patches (with interdiffs), it is easier for me to identify and understand the improvements.

(aside: merge request !41 looks a lot like the patch in #2 because I re-rolled it with minimal changes, and I wanted to take a look at the pipeline results before making further changes)

(assigning credit)

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.

Reviews welcome!

mparker17 → created an issue.

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