Problem/Motivation

We have this happening with entity_translation. This means two bugs: module_invoke_all('path_delete', $path); will be called with FALSE instead of an array, violating the API.

Also when drupal_clear_path_cache($path['source']); is called with $path = FALSE then, while you are not getting a warning (PHP WTF) but you are calling drupal_clear_path_cache(NULL); which enforces a whitelist rebuild. This is a (severe) performance bug.

Proposed resolution

Just get the hell out of dodge if the path already doesn't exist.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
path_delete.patch916 byteschx

Comments

chx created an issue. See original summary.

David_Rothstein’s picture

David_Rothstein commented

Possibly a duplicate of #1025904: path_delete() cannot handle multiple aliases and/or #2637570: path_delete() tries to delete non-existing path, leads to flushing whitelist cache?

chx’s picture

chx commented

No, this is a different bug, actually two: module_invoke_all('path_delete', $path); you will call this with FALSE instead of an array violating the API.

Also when you call drupal_clear_path_cache($path['source']); with $path = FALSE then, while you are not getting a warning (PHP WTF) but you are calling drupal_clear_path_cache(NULL); which enforces a whitelist rebuild. This is a (severe) performance bug.

chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented

Opsie, forgot to set attribute.

fabianx’s picture

fabianx commented
Priority: Normal » Major
Issue tags: +Performance

Yup, agree that this is a severe performance bug affecting D7 only.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Unless we need a test, I think this should be good to go.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Closed (duplicate)

I double-checked, and the patch at #1025904: path_delete() cannot handle multiple aliases fixes both the issues mentioned above (see also the issue summary there: For example, I create a node without a path alias. When I delete that node, path_node_delete() calls path_delete(array('source' => 'node/1')). Since path_load() returns FALSE, we get hook_path_delete(FALSE) invoked.)

Plus it already has tests.

I'm also not sure we'd want to add the TRUE/FALSE return value, since delete functions don't often do that?