path_delete() cannot handle multiple aliases

After applying the patch in #1, I get the following error:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 1: DELETE FROM {url_alias} WHERE (pid IN ()) ; Array ( ) in path_delete() (line 473 of /home/~/domains/d7multisite/includes/path.inc).

This happens when trying to delete a node that doesn't have a path alias. This kinda makes the whole Redirect module useless right now for some of my sites ;-)

See: #1041726: Error during node deletion with redirect_path_delete()

Patch tested, works, is good. (Without #1041726: Error during node deletion with redirect_path_delete() applied, of course).

Do tests need to be written, though?

Yeah, tests would be good to have here. This is a bug we don't want to have twice. :)

it works
thank you

I went to write a test for this, but I was unable to reproduce the error without this patch using drupal-7.0 or drupal-7.x-dev

My steps:
1. Created node
2. Added alias "alias1" to that node.
3. Added alias "alias2" to that node.
4. Deleted node

Upon deletion, both aliases are deleted.

Looking into the code, this is because the criteria for deleting the alias is the source path ("node/#"), which will match to both aliases. Under what conditions does this bug occur?

Attached is a web test for deleting a node with multiple aliases. As I have found, it should pass green without the original patch.

Asking bot to review test for point-in-case.

Thanks for the clarification. Thats significantly more complex than any of the test cases I have seen so far. Is there any standard of testing hooks? I will look around for any previous cases.

Edit: Found http://drupal.org/node/302577, although it doesn't state how to test the hook, and to call the test functions I need to somehow give the module the class...

Also, is there any reason to include or ignore the test I just wrote? Deletion of a single alias is tested, but not multiple. Would including this test be prudent, or bloat?

RoboPhred:

You can create a modules/path/tests directory and put a custom 'path_test' module there. Make sure to set hidden=TRUE in the path_test.info file. The module is enabled in the path.test routines using the setUp function.

Look in modules/filter/tests, or modules/node/tests, etc to see how to do it.

Current progress on adding tests. This patch is test only, and does NOT include the original.

Currently, the test for node deletion fails as it should. However, I am having issues with assertText not finding text that was inserted with drupal_set_message under some conditions... While the mock module outputs a message (confirmed by the test output), the actual test fails to find it. Someone please review this, its probably a mistake on my part, but I have been unable to find it so far.

Your patch will need to include the contents of modules/path/tests as well.

Thought that was being included. Looks like cvs diff won't actually pick up the contents of new files, while tortoiseCVS patch allows new files but garbles the file names and doesn't use the unified format. Until I track down a better way, here is tortoise's patch with the file paths modified.

Thought not. Turns out the d.org patch page on the handbook has info on this case that I overlooked before, this one should work.

Ok, so the "hook_path_delete not called" failure is expected as this patch does not include the fix, making hook_path_delete output "hook_path_delete called" in the menu. However, the other three should not be failing. The text they are looking for (such as "hook_path_delete: node/1 => pRrZo4pJ") are output in the messages area right next to the hook_path_delete message which is being identified as it should be. As far as I can tell this is a bug in the testing framework, but its possible its interpreting the text differently or there's some special sequence in there i'm not noticing. Can anyone confirm?

I think you have to use assertRaw() to get at text not in the content region. I'll upload a revised patch soon.

Two patches. The first is only the testing routines. The second incorporates Dave Reid's fix in #4.

Hopefully the test routines are easy enough to read -- they built upon RoboPhred's start in #17.

@pvm610: Please open a new issue as a feature request for your comment in #23. Thanks!

Rerolled the patch to the new core/ paths.

I think this may be resolved in 8.0.x. Logic has moved to AliasStorage::delete(), and also the path field type, PathItem. From AliasStorage::delete():

  /**
   * {@inheritdoc}
   */
  public function delete($conditions) {
    $path = $this->load($conditions);
    $query = $this->connection->delete('url_alias');
    foreach ($conditions as $field => $value) {
      $query->condition($field, $value);
    }
    $deleted = $query->execute();
    // @todo Switch to using an event for this instead of a hook.
    $this->moduleHandler->invokeAll('path_delete', array($path));
    return $deleted;
  }

That looks like the same logic in Drupal 8 as Drupal 7, so won't it have the same problem?

Note that #2637570: path_delete() tries to delete non-existing path, leads to flushing whitelist cache was recently filed which is related to this also (and is a possible duplicate).

I marked #2642848: path_delete() is broken for nonexisting paths as a duplicate also.

Adding the "Performance" tag and moving to major priority, based on discussion in that issue. (The performance aspect of this might only apply to Drupal 7 though.)

#1209226: Avoid slow query for path alias whitelists which is already applied to 8.x and needs review for 7.x gets rid of the slow query after a cache clear. However afaik the cache clear will still happen in 8.x.

Just got bitten by this issue with hook_path_delete and $path being empty. What needs doing on this one?

Changing back to 7.x as this was fixed as part of the path_alias entity conversion.

Ran into super slow node deletes today on a site with entity_translation module enabled. It implements path_entity_translation_delete() and tries to delete non-existing paths, which then triggers a slow drupal_path_alias_whitelist_rebuild() down the line. This patch fixes path_delete() to not call hooks on non-existing paths, will test that.

1. +++ b/core/includes/path.inc
   @@ -464,14 +464,13 @@ function path_delete($criteria) {
   +  $paths = array();
   

   unused variable that should be removed.

2. +++ b/core/modules/path/path.test
   @@ -546,3 +546,62 @@ class PathMonolingualTestCase extends PathTestCase {
   +class PathHooksTestCase extends DrupalWebTestCase {
   

   this test seems a bit complicated, we don't actually need to test the UI? We could just set variables when hooks are called. We don't even need to create nodes, we could just test with the /user path for example.

3. +++ b/core/modules/path/path.test
   @@ -546,3 +546,62 @@ class PathMonolingualTestCase extends PathTestCase {
   +  function setUp() {
   

   public function

4. +++ b/core/modules/path/path.test
   @@ -546,3 +546,62 @@ class PathMonolingualTestCase extends PathTestCase {
   +  function testPathHooks() {
   

   public function

Here is a rerolled patch. I'm a bit afraid of the endless while loop that could potentially run many iterations if there are many path aliases or somebody calls the function wrong. If a developer calls path_delete(array());by accident then it will wipe out your whole URL alias table!

On the other hand path_delete() currently also wipes out the url_alias table if called with en empty array, so the while loop might actually be ok?

Does not include the test case yet since I'm not really happy with it.

Related issue is #1209226: Avoid slow query for path alias whitelists as catch mentioned. The performance problem is less often hit with his patch here, so we should do both.

Noticed that clearing caches should happen AFTER the delete query. Invoking hooks should be done before the delete query, we do it the same way as with node_delete() for example.

Invoking hooks should be done before the delete query, we do it the same way as with node_delete() for example.

Indeed this is the case for hook_node_delete().
For paths, until now, the hook was invoked _after_ the deletion.
This could be considered a BC break. But since this hook was already broken to begin with, perhaps it is ok?

The only implementation I am currently aware of is redirect_path_delete().
Older versions of redirect had this code commented out, but it was re-introduced in #1763436: redirect_path_delete() code is commented, not creating redirect on alias delete.
The code in redirect itself seems like it does not care whether it is called before or after the alias is deleted.
But there could be modules which react to e.g. hook_redirect_insert() and do something with aliases.

I think we should at least provide good explanation why the hook should be called before the delete operation.

Or, if we want to maintain maximum BC, we could introduce an additional hook_path_pre_delete() which runs before, and keep hook_path_delete() running after.

Here is a rerolled patch. I'm a bit afraid of the endless while loop that could potentially run many iterations if there are many path aliases or somebody calls the function wrong.

We can get an infinite loop, if one hook implementation creates new paths which match the criteria for deletion.
To prevent this, we should load all matching paths at once, or (to save memory) iterate through a single select query instead of calling path_load().

path_load() is flawed in that it only ever loads the first matching result.
There could be an alternative to path_load() with generator syntax - or something equivalent for older PHP versions.

For paths, until now, the hook was invoked _after_ the deletion.
This could be considered a BC break. But since this hook was already broken to begin with, perhaps it is ok?

A silly example would be a module that implements hook_path_delete() to report the number of paths left in the database.
With this change, this would produce a different result.
Interestingly, this implementation would not be "broken" with the current implementation, because it would not care about the $path parameter being sent to it.

Thanks for the input, I think you are right.

New patch:
1) Query all paths with a SELECT first. That way we don't do 1 query per while loop iteration and this is faster. It also prevents theoretical infinite loops.
2) Invoke the hook AFTER the DB record for the path was deleted to preserve maximum backwards compatibility.

This patch does not apply anymore after #3084980: Trying to access array offset on value of type null in path_delete().

Our performance issue I mentioned is solved with that other issue, so we don't need this patch anymore. I think this issue is still useful for people that want to look into the problem that the hook is only invoked once even if multiple aliases have been deleted.