Problem/Motivation

The list of fields in the field mapping section of the media type form is not sorted.

Steps to reproduce

Proposed resolution

The $options should be sorted with natcasesort().

Remaining tasks

User interface changes

Before:
Click here to see the screenshots before the changes

After:
Click here to see the screenshots after the changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 drupal-field-mapping-options-3251647-5.patch1.74 KBdsdeiz
#4 drupal-field-mapping-options-3251647-3.patch1.69 KBdsdeiz
#3 drupal-field-mapping-options-3251647-3.patch1.65 KBdsdeiz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs tests

This will need some sort of automated test coverage.

dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz at Skvare commented
Status: Active » Needs review
FileSize
drupal-field-mapping-options-3251647-3.patch1.65 KB

Yeah, would this happen to work? Patch attached.

dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz at Skvare commented
FileSize
drupal-field-mapping-options-3251647-3.patch1.69 KB
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yup, looks good!

Thanks for the fix, and the tests too!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: -Needs tests
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
@@ -102,6 +102,12 @@ class MediaTypeCreationTest extends MediaJavascriptTestBase {
+    $options = $this->xpath('//select[@name="field_map[attribute_1]"]/option');
+    $this->assertEquals('- Skip field -', $options[0]->getText());
+    $this->assertEquals('Name', $options[1]->getText());
+    $this->assertEquals('Test source', $options[2]->getText());

Couple small things:

1. To prevent fatal errors, we should also do $this->assertGreaterThanOrEqual(3, count($options)); before asserting the text of the options.
2. We should use assertSame() instead of assertEquals().

Sorry to kick it back, but otherwise this looks great to me and I would be more than happy to re-RTBC when these are fixed. :)

dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz at Skvare commented
Status: Needs work » Needs review
FileSize
drupal-field-mapping-options-3251647-5.patch1.74 KB

Thanks! Patch attached.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

❤️ Thank you! Looks great. Back to RTBC we go!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

We don't sort form items that often, would it be possible to get a before/after screenshot here?

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

> We don't sort form items that often, would it be possible to get a before/after screenshot here?

It's the options in the SELECT element which is a list of fields.

We definitely should present SELECT options in a way that allows users to find them easily!

dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz at Skvare commented

Yeah, here's the example screenshots:

* Before: https://take.ms/LEtzV
* After: https://take.ms/iWDKl

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: drupal-field-mapping-options-3251647-5.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs screenshots

Unrelated test failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: drupal-field-mapping-options-3251647-5.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: drupal-field-mapping-options-3251647-5.patch, failed testing. View results

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at Portage CyberTech commented
Status: Needs work » Reviewed & tested by the community

Marking as RTBC as test failures are unrelated.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

@dsdeiz, thanks for the screenshots but having them off d.o means they can go away. I have encountered this problem when reviewing older issues for the Bug smash initiative. Plus before and after screenshots should be in the Issue Summary so that reviewer do not have to hunt for them in comments.

Setting to NW for an issue summary update to have screenshots in the Issue Summary.

Cheers,

thiagoht’s picture

thiagoht CreditAttribution: thiagoht at CI&T commented
Assigned: Unassigned » thiagoht
Status: Needs work » Active
thiagoht’s picture

thiagoht CreditAttribution: thiagoht at CI&T commented
Assigned: thiagoht » Unassigned
Issue summary: View changes
Status: Active » Needs review

I added the screenshots images links bellow the title User interface changes in summary as suggested by @quietone on comment #20.

Theses screenshots was related by @dsdeiz on comment #8.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Saving issue credit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed b3d036aacd to 10.1.x and 3d9eaebe71 to 10.0.x and 93a3045f5b to 9.5.x. Thanks!

Sorting makes sense to me here.

Will backport to 9.4.x once the branch is unfrozen...

  • alexpott committed b3d036a on 10.1.x 
    Issue #3251647 by dsdeiz, phenaproxima, joachim: Field mapping options...

  • alexpott committed 3d9eaeb on 10.0.x 
    Issue #3251647 by dsdeiz, phenaproxima, joachim: Field mapping options...

  • alexpott committed 93a3045 on 9.5.x 
    Issue #3251647 by dsdeiz, phenaproxima, joachim: Field mapping options...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Patch (to be ported) » Fixed

Committed ec84527 and pushed to 9.4.x. Thanks!

  • alexpott committed ec84527 on 9.4.x 
    Issue #3251647 by dsdeiz, phenaproxima, joachim: Field mapping options...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.