Please let modules know about the original URL alias in hook_path_update()

This would need to go into D8 first, no? Rerolling this for D8 would probably be a good novice task.

Also, could we get an automated test that tries to check for this?

rerolled for D8 and test added

justafish -- Thanks! Want to upload the test in its own pach as well, to demonstrate its failure on the testbot without the fix?

Oh, one other thing. The tests look great (insofar as I can review on my phone), but the API docs are not quite standard. See http://drupal.org/node/1354 for reference. For example, the summaries for classes, functions, and methods should be one line, 80 chars or less, that begins with a verb. Also, it's a but weird to call the one a unit test when it's a web test case. Can we use DrupalUnitTestCase? If not let's change the summary a bit.

I'll also take a look in Dreditor after breakfast.

Thanks!

Alright, so here are specific points for cleanup:

+++ b/core/modules/simpletest/tests/path.testundefined
@@ -333,3 +333,48 @@ class PathLookupTest extends DrupalWebTestCase {
+ * Unit test for path_save().
...
+   * Test that path_save() makes the original path available.

+++ b/core/modules/simpletest/tests/path_test.moduleundefined
@@ -0,0 +1,23 @@
+ * Reset/initialize the history of calls.

These should begin with third-person verbs.

+++ b/core/modules/simpletest/tests/path.testundefined
@@ -333,3 +333,48 @@ class PathLookupTest extends DrupalWebTestCase {
+  }
+  ¶
+  function setUp() {
...
+    path_save($path);
+    ¶
+    // Alter the path
+    $path['alias'] = 'bar';
+    path_save($path);
+    ¶

+++ b/core/modules/simpletest/tests/path_test.moduleundefined
@@ -0,0 +1,23 @@
+ */
+ ¶
...
+ ¶

Bits of trailing whitespace here.

+++ b/core/modules/simpletest/tests/path.testundefined
@@ -333,3 +333,48 @@ class PathLookupTest extends DrupalWebTestCase {
+    // Create a language neutral alias

There should be a period at the end of this comment. Also, minor grammar thing, but "language-neutral" should be hyphenated.

+++ b/core/modules/simpletest/tests/path_test.infoundefined
@@ -0,0 +1,6 @@
\ No newline at end of file

+++ b/core/modules/simpletest/tests/path_test.moduleundefined
@@ -0,0 +1,23 @@
\ No newline at end of file

Let's get a single newline at the end of these two files.

+++ b/core/modules/simpletest/tests/path.testundefined
@@ -333,3 +333,48 @@ class PathLookupTest extends DrupalWebTestCase {
+    // Create a language neutral alias
...
+    // Alter the path

Oh, and these two need to end in periods.

Sorry for all the noise!

Hi xjm, Thanks very much for the review! Hopefully I've fixed it all up..

Attached is the original patch rerolled for D8, the test to show it failing and the patch/test combined.

Thanks @justafish! Looks like you got most of it. Thanks also for uploading the test-only patch to expose the failures on testbot.

The only remaining things are found are all really minor code style/grammar things:

1. +++ b/core/modules/simpletest/tests/path.testundefined
   @@ -333,3 +333,49 @@ class PathLookupTest extends DrupalWebTestCase {
   +/**
   + * Tests path_save() function.
   + */
   

   This probably needs to be:

   Tests the path_save() function.

2. +++ b/core/modules/simpletest/tests/path.testundefined
   @@ -333,3 +333,49 @@ class PathLookupTest extends DrupalWebTestCase {
   +    // Enable dummy module that implements hook_path_update.
   

   I think we can probably omit this comment (I looked a bit and didn't seen any precedent for adding a comment like this with other tests.)

3. +++ b/core/modules/simpletest/tests/path.testundefined
   @@ -333,3 +333,49 @@ class PathLookupTest extends DrupalWebTestCase {
   +  /**
   +   * Tests path_save() makes the original path available to modules.
   +   */
   

   I think this needs to read:

   Tests that path_save() makes...

4. +++ b/core/modules/simpletest/tests/path_test.infoundefined
   @@ -0,0 +1,6 @@
   +name = "Hook path tests"
   +description = "Support module for path hook testing."
   +package = Testing
   +version = VERSION
   +core = 8.x
   +hidden = TRUE
   \ No newline at end of file
   

   This file is still missing its terminal newline.

5. +++ b/core/modules/simpletest/tests/path_test.moduleundefined
   @@ -0,0 +1,22 @@
   + * Dummy module implementing hook path.
   

   This should probably be:

   Dummy module implementing hook_path_update().

   Correct?

6. +++ b/core/modules/simpletest/tests/path_test.moduleundefined
   @@ -0,0 +1,22 @@
   + * Resets the path test results
   

   There should be a period at the end of this summary.

menu.test L113 and file.test L239 have comments pertaining to enabling dummy modules in their setUp functions. I actually found this very useful from menu.test when working out how to do this so I'm going to leave it in.

"Dummy module implementing hook path." is the exact same comment at the top of menu_test.module, but I've changed it to something more useful for future anyway.

New patches attached rerolled against head.

Attached adds a grammar fix to an inline comment:

Enable a helper module that implements hook_path_update().

Thanks @justafish!

I guess having a comment for the setUp() method is not consistent with most test cases that enable test-only modules, but if you think it's helpful, it doesn't hurt anything. :) I'll restrain my compulsion that we should put those inline comments in every other setUp() function if we do here. :P The comment at does at least need to be hook_path_update() with parens and have an article, like:

Enable a helper module that implements hook_path_update().

(Fixed in the patch above. Since these are just documentation cleanups it's okay to upload just a single patch for the subsequent rerolls, as we have the exposed failure in #13 to demonstrate the test for reviewers.)

I like the new file docblock; that is clearer as well.

Thanks for all your work on this. I think it's RTBC now.

#15: 998256-path-save-original-test.patch queued for re-testing.

#15: 998256-path-save-original.patch queued for re-testing.

#15: 998256-path-save-combined.patch queued for re-testing.

accounting for altered language key

Looks good. Committed/pushed to 8.x, moving back to 7.x.

D7 backport.

Backport is fine too.

+++ b/modules/simpletest/tests/path_test.infoundefined
@@ -0,0 +1,6 @@
+core = 8.x

Should be core 7.x. Right?

Hmmm how did that one slip through? Why would that pass testbot?

Yeah, definitely odd that that didn't get caught. Thanks @webflo!

I'm concerned about the testbot thing; it implies the test coverage is incomplete. We should try to debug why it didn't fail and either amend the test or open a followup issue, I think.

Alright, sun and chx clarified #31 on IRC. The core version compatibility isn't checked when simpletest enables a module, so it was being enabled properly in the test despite the .info file.

Thanks @xjm!

simpletest_knowledge++

Committed and pushed to 7.x. Thanks!