Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Title: Add remaining configuration entity/ page into configuration translation module » Add remaining configuration entity or page into configuration translation module
Status: Active » Needs review
FileSize
135.76 KB
40.8 KB
1.12 KB

Initial patch with screenshot...

vijaycs85’s picture

Issue tags: +Needs tests

Adding tag to extend test cases.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Issue tags: -Needs tests

Looking at this.

Gábor Hojtsy’s picture

Adding tests. Also moved around mappers in alphabetical order. I can live with that in this patch :)

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-4.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +sprint
FileSize
5.65 KB

Found in #2044389: [META] Fix broken configuration translation pages that picture mapping also need to be supported. That is however blocked on another core issue I just found, see #2044865: Picture mappings cannot be edited. Added the picture mapper anyway here. No tests for that yet, since that would require #2044865: Picture mappings cannot be edited to land to pass.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
909 bytes

The image style related fail is due to #1809376: Use EntityListController for image styles, the current image style listing operations are not possible to alter. So we'll need to not try to test for what cannot be there for a while.

Gábor Hojtsy’s picture

Add tests snippet for picture listing stuff.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-9.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
7.53 KB

The picture message assert was bad. Fixing that and adding date formats too :) This will fail as well due to #2038285: Update configuration schema for date formats as entities.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
7.64 KB

Make date format tests actually run. Uncomment failing test which we know is due to core issue. Add core issue link.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-13.patch, failed testing.

Gábor Hojtsy’s picture

So this uncovers that date format translate page does not work due to #2038285: Update configuration schema for date formats as entities, commenting that one out with todo as well.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.71 KB
883 bytes

Actual patch.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.73 KB
1.23 KB

There may be more failures around date stuff, let's comment this out for now.

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-rss-image-18.patch, failed testing.

Gábor Hojtsy’s picture

The notice was actually for the picture page which my core patch resolves, so I could not reproduce locally due to that patch applied. Reproduced now, commenting that out too.. Heh.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.86 KB
1.93 KB
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Committed this for now. The @todo's will be taken care of later. They are clearly marked.

Gábor Hojtsy’s picture

FileSize
3.16 KB

Some core fixes landed in the meantime. Let's try to activate those tests :)

Gábor Hojtsy’s picture

Status: Fixed » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Fixed

All right, committed and pushed that one.

YesCT’s picture

Status: Fixed » Needs work

we still have this @todo for #1809376: Use EntityListController for image styles So lets keep this open for that.

$ ag -B3 -A3 1809376 *
lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
322-    $this->drupalGet('admin/config/media/image-styles');
323-
324-    $translate_link = 'admin/config/media/image-styles/manage/medium/translate';
325:    // @todo this will only be wired up once https://drupal.org/node/1809376 lands.
326-    // Test if the link to translate the style is on the page.
327-    //$this->assertLinkByHref($translate_link);
328-

[edited to correct the in code link]

tstoeckler’s picture

That issue link is wrong, does anyone know where that is supposed to point?

Gábor Hojtsy’s picture

#1809376: Use EntityListController for image styles is the right one as pointed out by @YesCT. The last number got lost for some reason in her quoted code. AFAIS the codebase has the right issue number/link too. So not sure how it got removed there...

tstoeckler’s picture

Ahh, thanks. I should remember to actually *read* comments.... *slapsforehead*

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Image style controllers landed, so let's close the loop here :)

YesCT’s picture

Assigned: Gábor Hojtsy » YesCT

I'll do it. k?

YesCT’s picture

Status: Needs work » Needs review
FileSize
909 bytes

didn't test this locally.

YesCT’s picture

FileSize
194.84 KB

without the patch link

[edit: oops without, I just forgot to turn on config translation. so the translate link is showing, lets just see how the tests come back. Should we look at something else?]

with:
image-styles.png

:)

Status: Needs review » Needs work

The last submitted patch, 2044387-config-translation-imagestyles-33.patch, failed testing.

Gábor Hojtsy’s picture

Looks like a great patch! The fail is unrelated, but would ideally need to be fixed first, so we can ensure the module passes. Looks like the shortcut entity was renamed? Probably needs a quick new issue.

YesCT’s picture

YesCT’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Yay, committed this patch then! Should be possible to close this down.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.