🇭🇺Hungary @huzooka

Some inspirational spagetti code:

once('babel.pager_links', 'babel-form .pager-wrapper a[href]', element).forEach((pagerLink) => {
  const $linkElement = $(pagerLink);
  let href = $linkElement.attr('href');
  if (href) {
    const parts = href.split('?');
    if (parts.length > 1) {
      queryFragment = parts[1].split('#');
      let queries = queryFragment[0].split('&');
      if (queries) {
        const queriesCleaned = [];
        queries.keys().forEach((k) => {
          if (!['ajax_page_state', 'ajax_form', '_wrapper_format', '_drupal_ajax'].includes(queries[k].split('=')[0])) {
            queriesCleaned.push(queries[k]);
          }
        })
        queries = queriesCleaned.join('&');
        href = parts[0] + '?' + queriesCleaned.join('&');
        if (queryFragment.length > 1) {
          href += '#' + queryFragment[1];
        }
      }
    }
  }

  $linkElement.attr('href', href);
  $linkElement.classList.add('use-ajax');
});

huzooka → created an issue.

huzooka → created an issue.

huzooka → created an issue. See original summary → .

In 📌 Refactor translation UI/form Active , we see this behavior even without using PagerSelectExtender.

Adding the related core ticket:
🐛 Find a generic way to resolve race condition on AJAX change event and form submission Active

There is an open core issue about Views AJAX filters 📌 AJAXified Views should also change the URL by using history.pushState() Active which mentions Views AJAX History → module.

Maybe we should find (or create if not exists) a core ticket (feature request) about a History.pushState() AJAX command. And until it is resolved we can build our own.

The filter loss mentioned in #27 was caused by a race condition between two AJAX operation what I wasn't able to prevent. First operation was started by hitting ENTER; the second operation was started because the search field was focused out (because hitting ENTER). Rescoped the ticket so only clicking the filter apply button or hitting ENTER will apply the new filters.

You didn't see #20 to be resolved because you were expecting a message even if there were no changes done to the translation - now I also show a status message saying this.

Changes:

1. Addressed #19 in a way that seems obvious to me: not showing toolbar link on the translate form's path. Also I found some inconsistencies with the toolbar link – it was shown for users without the translate permission. Fixed them in this ticket.
2. Fixed #20. (BTW the message was shown, but unfortunately not in the modal; it was added to the page.)
3. Also added another workaround for #21: If an AJAX operation is already in progress, we don't let another to be started additionally. Ideally, an AJAX operation targeting a form element should disable all other interactive element but unfortunately it only disables the initiator element.
4. Added another test to cover the toolbar link and to test the translate form's basic functionality within the modal

Proposed follow-up tickets to be created (note for mostly myself):

1. History pushState AJAX command (see https://www.drupal.org/project/babel/issues/3533685#mr17-note562236 📌 Refactor translation UI/form Active )
2. Reset pager when filter is changed (see https://www.drupal.org/project/babel/issues/3533685#mr17-note562245 📌 Refactor translation UI/form Active )
3. Remove workarounds of 🐛 Fixed pagination and sorting of tables on pages with ajax Needs work when core issue is resolved
4. Remove workarounds of 🐛 Ajax callbacks on paginated forms with PagerSelectExtender not updating on first callback Active when core issue is resolved
5. Remove workarounds of 🐛 Find a generic way to resolve race condition on AJAX change event and form submission Active

Unfortunately we also hit 🐛 Error: Cannot read properties of undefined (reading 'settings')
 with dialog.position.js Active after the first AJAX operation performed within the modal dialog, so I also add the core issue as related.

Asking a second review from Adrian.

I propose to solve https://www.drupal.org/project/babel/issues/3533685#mr17-note562236 📌 Refactor translation UI/form Active (history pushstate AJAX command?) and https://www.drupal.org/project/babel/issues/3533685#mr17-note562245 📌 Refactor translation UI/form Active (pager reset on filter change) in dedicated tickets.

Also, if this ticket is accepted, I have to create 2 more follow-up tickets with some code removal if core bugs are resolved ( 🐛 Fixed pagination and sorting of tables on pages with ajax Needs work and 🐛 Ajax callbacks on paginated forms with PagerSelectExtender not updating on first callback Active )

Forgot to set the right assignee.

Filtering now works with AJAX-submit even if user hits enter;
Found a way to disable filter element refocus (this was very annoying when I was using pointer device).

I added both a non-JS and a JS test for the UI.

Asking for a first round of QA.

Re #13:

After a discussion with Claudiu, the status must belong to the actual string / context pair. So in this ticket, we must manipulate the status property from the babel_source_status table.

The status column in babel_source must be ignored as it will be removed in anouther ticket.

I found the root cause: https://git.drupalcode.org/project/babel/-/blob/1.x/src/BabelService.php...

Since strings can be (and are) reused across configs (moreover, a string used in locale translation can also be used in configs), hashes are more likely to cause collision since they already exist in this table. Just install a standard profile and then set up debugger at the caught exception to see them. If one record fails then the entire set fails (up to 200 records).

My proposal is to remove these "workarounds" because they aren't really workarounds (they cause data loss). The status insert query I'm referring can be replaced by an upsert; the first query in this BabelSercvice::update() method must be exectuted individually; or the data structure must be reordered to contain only one primary key (e.g.: "<plugin>:<babel-id>") so that we can use upsert there too.

1. drush si standard -y
2. drush en babel -y
3. drush sql:query "SELECT COUNT(*) FROM (SELECT DISTINCT hash FROM babel_source WHERE plugin = 'config') wrapper" ➡️ 288
4. drush sql:query "SELECT COUNT(*) FROM (SELECT DISTINCT bs.hash FROM babel_source bs INNER JOIN babel_source_status bss ON bs.hash = bss.hash WHERE plugin = 'config') wrapper ➡️ 78

API changes:

1. \Drupal\babel\Model\StringTranslation::$translation is not read-only anymore: Since translations can be changed and removed, it makes no sense do force ourselves to destroy the original and reinstantiate a replacement object on a translation change / deletion. This simplified the translation import form a lot, because we don't have to update the corresponding StringTranslation object at the array key in the form state storage.
2. \Drupal\babel\Plugin\Babel\TranslationTypePluginInterface::updateTranslation does not return anything anymore. Before my MR, it returned a StringTranslation object when a translation was added or modified, or just returned NULL if translation was deleted.

Functional changes:

1. Form wasn't functional without JavaScript - this is now fixed.
2. To make the translate form's new pager work without page refresh, I added a dedicated render element, a dedicated theme function and a dedicated route. In the meantime, I had to add workarounds for 🐛 Fixed pagination and sorting of tables on pages with ajax Needs work .
3. I'm still not satisfied with the filtering; I am thinking about creating separate form for it.

In this Babel MR, the logic which was added to control the status of the translatable strings is wrong: The code assumes that the $keys variable here contains a single plugin key, but it is actually a comma-separated string of babel_source IDs (same string / context pair might be used even by a Locale and also in multiple configuration).

I'd like to see the motivation behind this status property, mostly to see what it is about.

1. If status really belongs to a babel_source record, we must create another UI which lists those individual records. But, we can also delete the babel_source_status table because it does not make any sense.
2. If status belongs to an actual source string + context pair then most probably we should remove it from the babel_source table.

Reverted the commit and at the same time, added the "Needs issue summary update" tag.

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

huzooka → created an issue.

huzooka → created an issue. See original summary → .

I finally managed this to happen.

1. Added a UI test for export / import CSV
2. Added test coverage for menu link content translation type plugin
3. Added test coverage for the helper service of Babel Menu Link Content
4. Fixed some minor preexisting bugs.

huzooka → created an issue.

huzooka → changed the visibility of the branch 3535370-babel-source-wrong-delete to hidden.

I just wasn't able to find out the situation when we have to remove things from babel_source after a locale rebuild, so I created 🐛 Locale translatables with non-javascript source are removed from Babel table (wrong JS alter logic) Active .

huzooka → created an issue.

The committed solution is overcomplicated, makes initial page load (with cold caches) slow and e.g. strings are added twice (maybe because of a bad merge)

Parent ticket was merged.

Created the follow-up 📌 [PP-1] Add test coverage for babel_menu_link_content and functional test for export / import Postponed .

huzooka → created an issue.

To keep both my momentum and also let my teammates benefit from the checks "added" by this ticket Claudiu and me aggreed on that I can add the e2e test for the export / import and the test coverage for the menu link content translator plugin in a separate, follow-up ticket.

So, I ask for the first review here.

I create that follow-up asap.

huzooka → created an issue.

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

@kristiaanvandeneynde Thank you!
If nobody picks this up I can start working on this tomorrow.

huzooka → created an issue.

Lots of coding standard violations; real Drupal 11 compatibility is still not here (a temporary Drush version constraint was added to block every version above 13.0.0-beta4), tests are failing on PHP 8.4....

But with PHP 8.3, you can actually use this again!

I will create a separate ticket to handle the Drush thing but after it's resolved, I can do a new release.

huzooka → created an issue.

Resolved in 📌 Automated Drupal 11 compatibility fixes for sel Needs review

huzooka → changed the visibility of the branch project-update-bot-only to hidden.

There wasn't any issue with the module - only the test was wrong.

huzooka → changed the visibility of the branch 3433452-automated-drupal-11 to hidden.

huzooka → changed the visibility of the branch project-update-bot-only to hidden.

huzooka → changed the visibility of the branch 3433452-automated-drupal-11 to active.

huzooka → changed the visibility of the branch 3433452-automated-drupal-11 to hidden.

huzooka → created an issue.

huzooka → created an issue.

huzooka → created an issue.

huzooka → created an issue.

huzooka → created an issue.

huzooka → created an issue.

Re #5,
Thank you for testing this! I also addressed the issue you found, and added it as a test case.

I'm asking for a second round review.

Created a patch on top of 6.2.x.

I addressed review feedback, created 📌 Convert FieldConfigFormAlter and BundleEntityFormAlter to a single service Active as a follow-up; also added some comments for our future selves.

huzooka → created an issue.

huzooka → changed the visibility of the branch 3362124-3.x to hidden.

I was wondering why this issue only affects horizontal tabs - and found out that in case of vertical tabs, Drupal core handles this in misc/vertical-tabs.js with handleFragmentLinkClickOrHashChange(). So I changed the approach, and added the code to 3362124-4.x branch. I think that's the right way to solve this issue.

Could you please check this new MR (https://git.drupalcode.org/project/field_group/-/merge_requests/110) before I align also the other one for 3.x ?

We also need this feature. Our only problem is that the selectors used in Javascript are part of the markup delivered by the error message theme functions / elements (also JS!: Drupal.theme.message), and we need this to work in multiple themes simultaneously.

I want to work on this a bit to make it theme-agnostic.

huzooka → created an issue.

I also created a patch which can be applied on Webform 6.2.9; uploading it here if anyone is interested.

CS and Eslint violations are preexisting imho.
Asking for a (quick) review and about further tasks.

huzooka → created an issue.

Checked and also tested !15, I found no problems, it's perfect.

To me the widget config looked a bit weird when I first saw the exported display config, but actually it also makes sense - the first level "settings" key belongs to Drupal's form widgets, but the second "settings" key is actually a Slim Select (object) constructor property, like here https://slimselectjs.com/settings#openPosition

...
field_tags: 
  type: slim_select
  weight: 3
  region: content
  settings:     
    settings:       
      allowDeselect: true
      closeOnSelect: false
      openPosition: top
  third_party_settings: {  }
...