🇵🇱Poland @jsobiecki

The 2.x now includes really basic Kernel test. I'll try to find some time and update it with more valuable test suite.

Hello @niharika.s,

What version of module you are using? I believe that the module will be listed on update listed only if there is pending update to it.

Hi @murilohp,

Good catch! I'm happy to merge the MR, please solve conflicts first.

I deployed fix for missing URL. Tested on D9 manually. I also scanned LinkIterater through phpcs / phpstan tools.

@ana.maria.andrei - thank you reporting this. I'm looking on your patch. Strange thing, maybe I didn't commit everything from my workspace

I pushed fix for the issue on D9. I also added very basic (just loading of service) Kernel test, hopefully it will be start for test suite that will solve problems like this. It's late for me today, but tomorrow I'll try do some additional tests and I'll push the next release that works on both D9 and D10.

OH! Thanks for reporting. I'll deploy fix today later

Hello @aalin,

This patch doesn't apply.

Hello @canardesign,

I failed to reproduce this issue. Are you still able to see same problem?

Looking good! merged

Looking good, thanks!

jsobiecki → created an issue.

I really like this change! The code looks much simpler and the fact that we reduced SQL queries number of site is very important.

I share concern of @Anybody, that we may fall into some of regressions and unit tests can help reduce that risk. But I'd create a initial unit (rather kernel) test as separate tasks. This module was designed as quick and simple workaround in D8 times and I wanted to keep it that way. As we have D11 and it's still helpful, it's good idea to improve its quality. But not now, I'll do that as separate issue.

I did some small refactor of code, as patch failed to apply and did manual testing. I was no able to "break" module after this modification.

Ok, fix merged. Thanks everyone!

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

Change commited. Thank you all people involved in this thread! I updated last patch with removal of deprecated (in D10) code for exception handling and replcaed it with proper solution for D10 and latter....

@dshields,

Thank you for your patch. I merged your changes, and added small improvements (description include iinformation about tokens that may be used in field). Merged to 2.x branch, thanks again!

Hello @ibustos,

Thank you for your patch. Seems fine, but please fix the merge conflicts first.

Hello @Rajiv,

Thank you for your feedback. I was trying to reproduce the issue on vanilla D10 installation with two languages enabled.

However, I was unable to reproduce it. This module adds a different field, so I'm not able to see what relation it has with the translatable menu link module.

Take a look on expected behaviour:

Do you use the last stable version?

Hello Eduardo,

Thank you for your patch. Unfortunately, I need to put it on pause. The reason behind this is that you are using code that is not part of Drupal core. The fragments you are using are still in development at the moment ( https://www.drupal.org/i/3091246 → ) and needs work.

Generally, I like the approach as it axe's some old hooks, but since it's not yet part of core, I'd rather wait.
Maybe good idea would be to start release 3.x after the dependent change will be part of both 10.x and 11.x branches of Drupal. I think axing 9.x and 8.x is not a bad thing at this moment.

MR merged, will be part of next minor release :)

Nice idea. I'll test it and if it will work correctly, happy to commit.

I'll try to update documentation for project, as it's a feature that is not that obvious.

Hello @dqd,

I re-created an empty, Drupal test site with similar setup (two domains, domain-based language detection).

For testing, I'm using the dev version of the module.

My results:

"Vanilla" dev version - problem persists; the get-data endpoint returns a 403 error, and no suggestions appear in the coffee widget.
"Vanilla" dev version with automatic D11 fixes patch (https://git.drupalcode.org/project/coffee/-/merge_requests/15.diff ) - problem persists; the get-data endpoint still returns a 403 error, and there are no suggestions.
However, "Vanilla" dev version with automatic D11 fixes and the patch from this issue apply cleanly, and the problem is solved; the widget works as expected.

As a bottom line, the problem still exists, but the patches apply cleanly and fix the reported issues. The issue was detected on a really "clean" installation of Drupal 10.

I created the fix for reported issue. Instead of using placeholder() method, I decided to generate SQL query and use regular expression to count all suffixes. The project doesn't have regular test suite, but at my test cases this seemed to fix the issue.

I'm affected by this problem, and unfortunately patch #13 doesn't work. After some debugging, my impression is that root cause of issue is usage of placeholder() method in ArgOrderStort plugin:

    // Attempt to determine the number appended to the query substitutes.
    // Calling the placeholder function increments the placeholder count by 1,
    // so we subtract one to get the number that was used before.
    $placeholder_suffix = str_replace(':' . $left_table . '_' . $left_field, '', $this->query->placeholder($left_table . '_' . $left_field));
    if (!empty($placeholder_suffix) && intval($placeholder_suffix) > 1) {
      $placeholder_suffix = intval($placeholder_suffix) - 1;
    }

The placeholder generates a field placeholder name with dynamic suffix (number). It uses static variable to provide proper number for suffix. Each execution of placeholder() the method is increasing suffix for sql placeholder.

My scenario is that I have few views that are using same base table. As static variable is not being restarted for each view, the placeholder is increased and provides false (from the views_arg_order_sort) arguments. I'll try to rewrite the code to avoid placeholder() method. It generates unexpected side effects.

jsobiecki → created an issue.

The MR includes simple fix. The fix assumes

1. CORS requests allowed between en.site.com and site.com
2. Cookie has samesite: None and cookie_domain is ".site.com"

jsobiecki → created an issue.

The new 2.1.0 compatible with D10 has been released. Thank you all for all the interest shown! My contact channels were bombed by requests to finally do that :)

Merged last patch, hopefully it will fix old issues.

It's looks like this is a bug that haunts this module for people that tried to update from old version. I'm not able to reproduce this issue anymore, but change provided by @rcoding seems to be ok and not harmful. I'm going to merge it.

The MR includes proposed solution.

jsobiecki → created an issue.

Hi rastepanyan, djmymy,

Could you provide me your execution context (What version of Drupal core, PHP, and MySQL). It seems that update 9002 wasn't able to update database schema for you. On my end, it worked as expected.

@djmymy,

Thanks for proposal, but I don't think it's walid way to handle entity schema updates. Please review hook 9002 from merge request: https://git.drupalcode.org/project/translatable_menu_link_uri/-/merge_re...

Maybe there is something missing there, after getting full context from you, I'll take a second glance on it.

Hi @rastepanyan,

I believe there is small misunderstanding. The patch I linked above (https://git.drupalcode.org/project/translatable_menu_link_uri/-/merge_re...) changes translatable_menu_link_uri_update_9003 function and adds translatable_menu_link_uri_update_9004 function. 9003 make sure that new structure is used, and 9004 is generaly what old 9003 was.

If after applying patch you see only one (9004) update, please do following thing: (Drupal >= 9.3)
drush ev "\Drupal::service('update.update_hook_registry')->setInstalledVersion('translatable_menu_link_uri', 9002);"

this will allow you to re-execute update.

@rastepanyan Please apply changes from MR provided in https://git.drupalcode.org/project/translatable_menu_link_uri/-/merge_re... and try again.

Hi @djmymy,

Thanks for checking. It's weird, as I'm not able to reproduce this problem. I checked 9.x and 10.x Drupal versions. My scenario:

1. Install Umami profile as bootstrap.
2. Install old (1.x) version of translatable_menu_link
3. Configure module as described in project description.
4. Create couple menu links.
5. Switch to translatable_menu_link_uri-3259442-3259442-notice-undefined-index
6. drush updatedb:

The following updates are pending:

translatable_menu_link_uri module : 
  9002 - Make sure that database and entities definition are in sync.
  9003 - 

Do you wish to run all pending updates? (y/n): y
Performing translatable_menu_link_uri_update_9002                                                                  [ok]
Performing translatable_menu_link_uri_update_9003                                                                  [ok]
Cache rebuild complete.                               

So I have couple of questions:
- What Drupal core version you use?
- Could you reproduce my steps?

Hello @djmymy The fix is not been commited yet. There should be two updates on your list, I see only one.

Please make sure you applied this patch: https://git.drupalcode.org/project/translatable_menu_link_uri/-/merge_re....

If updatedb will not see both updates, you may rename function names and bump numbers being part of function names (eg. 9003 => 9005 and 9004 to 9006).

Please let me know if it helped.

Hi @anybody. I'd like to commit changes from https://www.drupal.org/project/translatable_menu_link_uri/issues/3259442 🐛 Notice: Undefined index: link_override__title in Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromSharedTables Fixed first. After that, I'll create new release.

Hi @antoineh,

I believe this problem doesn't exist anymore on 2.x branch. Are you able to confirm it?

Hi guys, I redundantly created MR for different bug that fixes different bug ( https://www.drupal.org/project/translatable_menu_link_uri/issues/3259442 🐛 Notice: Undefined index: link_override__title in Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromSharedTables Fixed ) by fixing entity mismatch issue. So probably this patch will not gonna be merged, as the one in this ticket is redundant. I overlooked this issue when worked on #3259442.

Thank you for all your work, I'll add people involved here to credits when I'll merge fix for #3259442.

Hi, I just opened MR against 2.x branch. It should provide update from 1.x to 2.x version. Please review if it's fixing your issues. If everything will be OK, I'll merge it in 2.x.

Hmm, I tried to reproduce problem - and it seems that indeed, 1.x to 2.x path is broken. I was able to run into some errors regarding field doesn't exist.

I wasn't able to reproduce. Waiting for further feedback.

Thanks all for contribution. I can confirm it works good. I decided to leave isset instead of empty.

Hi, Im doing final review as part of Global Contribution Weekend.

Good catch. I'll do a final review as part of Global Contribution Weekend.

Thanks! Looks all OK. I'll commit it shortly.

I slightly refactored @plopesc patch and added more verbose comment to implementation. I'll commit it to 2.x branch shortly.

Thank you all for contributing!

Hello, I'm working on this ticket as part of Drupal Global Contribution Weekend 2023.

I really like this UX improvement. I'll check if it's working OK and add this short code comment (thanks @Anybody) for you.

1. I believe that 8.x is not supported anymore, so I decided to skip this version
2. I tested on D10, with newest version of default content module - PASS
3. I tried to reproduce problem with TMGMT - PASS

I wasn't able to create NULL value at database, but I agree that fixing serialization is good thing.

Patch looks ok, decided to commit it.

Thanks all for contributing!

Hi All, thank you for reviewing patch and providing patches. I'm working on final review as part of GlobalContributionWeekend2023.

Thanks all for checking it, code in 2.x will be part of next release.

All seems to work correctly. Patch is just about to modify manifest of module. Seems to work well on vanilla D10 installation with two languages enabled. I'm committing it to 2.x branch.

Hi guys, thank you for your tests! I'm checking this patch as part of Drupal Global Contribution Weekend 2023