Current D7 default settings:

Current D8 default settings:

CommentFileSizeAuthor
#14 interdiff.txt1.75 KBdermario
#14 strings_to_remove-2656958-12.patch5.97 KBdermario
#10 interdiff.txt3.1 KBdermario
#10 strings_to_remove-2656958-10.patch4.99 KBdermario
#8 strings_to_remove-2656958-8.patch1.89 KBgaurav.goyal
#8 pathauto_code.png36.13 KBgaurav.goyal
#6 strings_to_remove-2656958-6.patch1.23 KBgaurav.goyal
#4 2656958-fix-default-ignore-words.patch1.26 KBDave Reid
#2 2656958-fix-default-ignore-words.patch547 bytesDave Reid
Selection_337.png10.91 KBDave Reid
Selection_338.png12.54 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid created an issue. See original summary.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid at Lullabot commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2656958-fix-default-ignore-words.patch547 bytes

Appears we had duplicate lines in the default config. And now we'll need tests to ensure this works out of the box.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid at Lullabot commented
Title: Strings to remove does set properly » Strings to remove default setting is incorrect
Issue tags: +Needs update path
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid at Lullabot commented
Issue tags: -Needs update path
FileSize
2656958-fix-default-ignore-words.patch1.26 KB

With update hook, still needs tests.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

Needs a reroll as we have new update functions.

gaurav.goyal’s picture

gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commented
FileSize
strings_to_remove-2656958-6.patch1.23 KB

Patch Rerolled. Updating status to needs review so that bots can review the patch.

gaurav.goyal’s picture

gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commented
Status: Needs work » Needs review
gaurav.goyal’s picture

gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commented
FileSize
pathauto_code.png36.13 KB
strings_to_remove-2656958-8.patch1.89 KB

One of the test for pathauto_cleanstring() function still uses old configuration as default value. I think we missed to update this.

Pathauto Test Code

Attached patch contains the update.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

What about using installConfig('pathauto') to install the default config and check with those settings? then we have that covered.

dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario at Unic commented
FileSize
strings_to_remove-2656958-10.patch4.99 KB
interdiff.txt3.1 KB

I attached the the changes to the settings form webtest, to ensure the changes introduced by patch in #8 will not break the tests introduced by https://www.drupal.org/node/2674494

This test will also check if the default values in the form are set correctly.

What about using installConfig('pathauto') to install the default config and check with those settings?

Do you mean we should use $this->installConfig(array('pathauto')) in PathautoUnitTest::testCleanString() instead of using $config->set() ?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yes, that's exactly what I meant. Assuming the default configuration actually matches what we save there, if not, then comment so and we'll ignore that, at least for this issue.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario at Unic commented
Assigned: Unassigned » dermario
dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario at Unic commented
Status: Needs work » Needs review
FileSize
strings_to_remove-2656958-12.patch5.97 KB
interdiff.txt1.75 KB

I gave it a try to refactor the test to use installConfig() and completely rely on settings in pathauto.settings.yml

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice, I like that.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

  • Berdir committed e015503 on 8.x-1.x authored by dermario 
    Issue #2656958 by Dave Reid, dermario, gaurav.goyal: Strings to remove...

Status: Fixed » Closed (fixed)

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