Should be possible to disable patterns (and not delete them)

Requires a bit more code than I initially expected. We need two confirmation forms, core unfortunately doesn't have defaults.

I'm also adding config_export keys as they were missing and added status to the lookup keys. That requires

I think this patch also needs reroll. Adding Needs reroll tag.

Attached is the rerolled patch.

The patch in #7applies well. Thank you! I reviewed the code and tested its functionality. Please see the attached Screens. Looks ok from my side, now i will investigate in writing some tests.

Here are some tests that check if the pattern workflows are working correctly.

Nice work, feedback below.

1. +++ b/src/Tests/PathautoPatternsWebTest.php
   @@ -0,0 +1,181 @@
   +/**
   + * Tests pathauto settings form.
   + *
   + * @group pathauto
   + */
   +class PathautoPatternsWebTest extends WebTestBase {
   +
   +  use PathautoTestHelperTrait;
   

   Wondering if we really need a separate test class from PathautoUiTest, especially since they have pretty much the same set up.

2. +++ b/src/Tests/PathautoPatternsWebTest.php
   @@ -0,0 +1,181 @@
   +   */
   +  function testPatternAddForm() {
   +    // On this page is some ajax magic when selecting a type, so we just can
   +    // check the default form values before the ajax call.
   +    $this->drupalGet('/admin/config/search/path/patterns/add');
   

   You might be interested in #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary.

   It actually got committed today but then reverted again due to problems with contrib tests.

   However, once this is in, we could actually write real JS tests for this.

   Also, you can actually already do write a test that works for this form. See \Drupal\pathauto\Tests\PathautoLocaleTest::testLanguagePatterns.

3. +++ b/src/Tests/PathautoPatternsWebTest.php
   @@ -0,0 +1,181 @@
   +  /**
   +   * Test the edit, disable, enable and delete workflows.
   +   */
   +  function testPatternWorkflows() {
   +    $pattern = $this->createPattern('node', '/testing/[node:title]');
   +
   

   If you change the add so that it works as in the referenced example, then I'd prefer to merge this into a single test method. Test methods add quite a bit of overhead in web tests

4. +++ b/src/Tests/PathautoPatternsWebTest.php
   @@ -0,0 +1,181 @@
   +
   +    // Reload pattern from storage and check if its disabled.
   +    $pattern = PathautoPattern::load($pattern->id());
   +    $this->assertFalse($pattern->status());
   +
   

   The one thing we don't have covered now is actually checking if a disabled pattern is considered for aliases or not.

   We can either do that here in the same UI test and create a node through the UI or API or we can extend PathautoUnitTest and somehow test it there.

Great feedback, thank you! I will go for it today.

Here is a new patch. These are the major changes:

I completely removed src/Tests/PathautoPatternsWebTest.php and moved the logic there into src/Tests/PathautoUiTest.php and src/Tests/PathautoUnitTest.php.

I extended the former PathautoUiTest::testPatternsValidation(), now PathautoUiTest::testPatternsWorkflow() with edit, enable, disable and delete tests and how nodes behave with enabled/disabled patterns.

PathautoUnitTest::testPatternStatus() tests how Nodes behave with enabled, disabled Patterns and when Nodes are saved and edited.

Ok, here is a new patch with (hopefully) working tests. I forgot to clear the pathauto caches \Drupal::service('pathauto.generator')->resetCaches();

The description in #13 is still up to date.

My interdiff refers to the patch in comment #10

Looks good, just one thing to improve.

1. +++ b/src/Tests/PathautoUiTest.php
   @@ -104,6 +106,74 @@ class PathautoUiTest extends WebTestBase {
   +    // Create a node with pattern disabled and check that we have no new alias.
   +    $title = 'Page Pattern disabled';
   +    $alias = '/page-pattern-disabled';
   +    $node = $this->createNode(['title' => $title, 'type' => 'page']);
   +
   +    $this->drupalGet($alias);
   +    $this->assertResponse(404);
   +    $this->assertNoEntityAlias($node, $alias);
   

   the second argument of assertNoEntityAlias() is the language, not the alias.

   It checks there there is no alias at all, which is what we want. But right now you actually check for the language '/page-pattern-disabled', which of course doesn't exist. Just remove that.

   I would also remove the drupalGet() and $alias completely, we don't want to hardcode how it would be when it doesn't exist.

2. +++ b/src/Tests/PathautoUnitTest.php
   @@ -461,6 +461,40 @@ class PathautoUnitTest extends KernelTestBase {
   +    // Create a new node with disabled pattern and make sure there is no new
   +    // alias.
   +    $title = 'Pattern disabled';
   +    $alias = '/content/pattern-disabled';
   +    $node2 = $this->drupalCreateNode(['title' => $title, 'type' => 'article']);
   

   same here.

Ahh i got tricked by assertEntityAlias() -signature, which is different. This and the other comments in #17 are applied now.

There is one thing i didn't get running and actually don't know if its a bug or of its relevant. Maybe its just a lack of knowledge from my side.

In PathautoUnitTest::testPatternStatus() i wanted to check if an existing Node ($node2) without an alias gets an alias after the pattern was enabled and $node2 was saved. Although i could see the alias was created and stored in database.

    // Enable the pattern again, save the node again and make sure there is a
    // new alias.
    $pattern->setStatus(TRUE)->save();

    $node2->path->pathauto = PathautoState::CREATE;
    $node2->save();
    // This assertion will fail, because the url is still "node/X"
    $this->assertEntityAlias($node2, $alias);

As i said before, i removed that code again because i am not sure if that case is relevant.

The previous test was failing because i created a duplicate pattern in my test. Now it should work.

Looks good to me, thanks.

Committed and pushed, thanks everyone for the help.