Adds a real api - path_load, path_save, path_delete.
I'm setting needs review cause i need a review of the API. tests forthcoming.

Files: 
CommentFileSizeAuthor
#69 drupal.path-inc-api.patch5.3 KBsun
Passed: 14394 passes, 0 fails, 0 exceptions
[ View ]
#66 drupal.path-api-major-fixes.66.patch29.8 KBsun
Failed: Failed to apply patch.
[ View ]
#60 drupal.path-api-major-fixes.60.patch29.65 KBsun
Failed: Failed to apply patch.
[ View ]
#58 332333_path-api-major-fixes.patch25.88 KBandypost
Failed: Failed to apply patch.
[ View ]
#57 drupal.path-api-major-fixes.57.patch26.75 KBsun
Failed: Failed to apply patch.
[ View ]
#55 drupal.path-api-major-fixes.55.patch25.24 KBsun
Failed: Failed to apply patch.
[ View ]
#44 drupal.path-api-major-fixes.patch18.42 KBsun
Failed: 13252 passes, 23 fails, 5 exceptions
[ View ]
#41 332333_path_fix.patch10.41 KBandypost
Failed: Failed to apply patch.
[ View ]
#37 drupal.path-api-cleanup.patch5.12 KBsun
Failed: 13838 passes, 19 fails, 0 exceptions
[ View ]
#35 path_api.patch34.69 KBa25i
Failed: Failed to apply patch.
[ View ]
#30 path_api.patch36.47 KBdmitrig01
Failed: Failed to apply patch.
[ View ]
#27 path_api_4.patch35.31 KBdmitrig01
Failed: Failed to apply patch.
[ View ]
#21 path_api.patch35.31 KBdmitrig01
Failed: Failed to apply patch.
[ View ]
#19 path_api.patch35.16 KBdmitrig01
Failed: 13861 passes, 0 fails, 27 exceptions
[ View ]
#16 path_api.patch35.08 KBdmitrig01
Failed: 13821 passes, 2 fails, 27 exceptions
[ View ]
#12 path_api.patch34.21 KBdmitrig01
Failed: Invalid PHP syntax in modules/path/path.module.
[ View ]
#10 path_api.patch33.74 KBdmitrig01
Failed: 13809 passes, 25 fails, 55 exceptions
[ View ]
#6 path_api.patch31.3 KBdmitrig01
Failed: 13602 passes, 36 fails, 40 exceptions
[ View ]
Serously guys, we need a real api for path module18.13 KBdmitrig01
Failed: Failed to apply patch.
[ View ]

Comments

Status:Needs review» Needs work

Do, or do not. There is no 'try.' If you want to rename dst and src then rename them throughout core.

Priority:Critical» Normal

tagging

Version:7.x-dev» 8.x-dev

Assigned:dmitrig01» Dave Reid

I am sad that we didn't get this accomplished sooner. Not having a proper API for path aliases is going to kill us in pathauto.module. Of course I don't have anyone else to blame but myself. I'm going to go ahead and lead this issue (sorry dmitrig01) to get the proper APIs in D8 now.

StatusFileSize
new31.3 KB
Failed: 13602 passes, 36 fails, 40 exceptions
[ View ]

This patch
- Adds real api functions
- Converts src and dst to source and alias because we don't abrv8

Status:Needs work» Needs review

and needs review (and testingbot's input)

Version:8.x-dev» 7.x-dev

Oops

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.74 KB
Failed: 13809 passes, 25 fails, 55 exceptions
[ View ]

This patch should pass almost all tests except for two fails which I'm having an impossible time tracking down.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new34.21 KB
Failed: Invalid PHP syntax in modules/path/path.module.
[ View ]

much improved

Also, this patch gets rid of the beast that is path_set_alias

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Woot! Keep 'em coming dmitrig01! I'll review once it gets a green from the testbot!

StatusFileSize
new35.08 KB
Failed: 13821 passes, 2 fails, 27 exceptions
[ View ]

Perhaps without a parse error this time.

Status:Needs review» Needs work

The last submitted patch failed testing.

This seems to resolve the test:

@@ -215,6 +215,9 @@ function path_node_update($node) {
   if (user_access('create url aliases') || user_access('administer url aliases') && isset($node->path)) {
     if (!is_array($node->path)) {
       $node->path = array('alias' => $node->path);
+      if (isset($node->pid)) {
+        $node->path['pid'] = $node->pid;
+      }
     }

Status:Needs work» Needs review
StatusFileSize
new35.16 KB
Failed: 13861 passes, 0 fails, 27 exceptions
[ View ]

Grendzy's solution in patch form

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new35.31 KB
Failed: Failed to apply patch.
[ View ]

with less (no?) exceptions

Status:Needs work» Needs review

At a high-level, this looks like a nice clean-up. Can't do an in-depth review right now though.

Assigned:Dave Reid» dmitrig01

Issue tags:+D7 API clean-up

Adding appropriate tag

Status:Needs review» Needs work
Issue tags:-D7 API clean-up

This looks great. Tests would fail dismally if this introduced bugs, path module is pretty well tested. Just found this:

+++ modules/path/path.module 14 Oct 2009 17:51:11 -0000
@@ -72,79 +73,82 @@ function path_menu() {
+}
+/*
+ *
+ * @param $path
+ *   A path array containing the following keys:

Missing one line summary.

I'm on crack. Are you, too?

Status:Needs work» Needs review
StatusFileSize
new35.31 KB
Failed: Failed to apply patch.
[ View ]

Priority:Normal» Critical
Issue tags:+API clean-up, +D7 API clean-up

First and foremost: We absolutely need hook invocations for page_insert, path_update, and path_delete, so please squeeze them in.

Aside from that, let's look at the details:

+++ modules/path/path.admin.inc 14 Oct 2009 17:51:11 -0000
@@ -140,37 +146,42 @@ function path_admin_form($form, &$form_s
/**
- * Save a new URL alias to the database.
+ * Save a URL alias to the database.
  */
function path_admin_form_submit($form, &$form_state) {
...
+  $path = array();
+  foreach (array('source', 'alias', 'pid', 'language') as $key) {
+    if (isset($form_state['values'][$key])) {
+      $path[$key] = $form_state['values'][$key];
+    }
+  }
+  path_save($path);

You could avoid that if #594650: Provide central $form_state['values'] clearance was in...

Just mentioning.

+++ modules/path/path.admin.inc 14 Oct 2009 17:51:11 -0000
@@ -180,15 +191,16 @@ function path_admin_form_submit($form, &
+function path_admin_delete_confirm($form, &$form_state, $path) {
...
+    return confirm_form(
+      $form,
+      t('Are you sure you want to delete path alias %title?',
+      array('%title' => $path['alias'])),
+      isset($_GET['destination']) ? $_GET['destination'] : 'admin/config/search/path'
+    );

As of today - YAY! - #582456: Make confirm_form() respect query string destination

You can drop that $_GET munging. :)

+++ modules/path/path.module 14 Oct 2009 17:51:11 -0000
@@ -72,79 +73,82 @@ function path_menu() {
+function path_load($criteria) {
...
+}
+/*

Missing blank line.

+++ modules/path/path.module 14 Oct 2009 17:51:11 -0000
@@ -72,79 +73,82 @@ function path_menu() {
+/*
+ *
+ * @param $path
...
+function path_save($path) {

1) Missing PHPDoc summary.

2) Wrong multiline comment syntax.

+++ modules/path/path.module 14 Oct 2009 17:51:11 -0000
@@ -72,79 +73,82 @@ function path_menu() {
+

Duplicate newline.

This review is powered by Dreditor.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.47 KB
Failed: Failed to apply patch.
[ View ]

Implemented sun's suggestoins

Status:Needs review» Reviewed & tested by the community

I suggested to leave the function body of those hook documentation examples empty, because it's very hard to think of valid examples that deal with paths.

I committed #594650: Provide central $form_state['values'] clearance so path_admin_form_submit() could be cleaned up a bit.

Status:Reviewed & tested by the community» Needs work

Also, when I run the database update, I got: Failed: 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 'NOT NULL DEFAULT 0' at line 1.

Yeah, pretty sure we want to keep those as varchars and not restrict ourselves to integer paths. :)

+  db_change_field('url_alias', 'src', 'source', array('varchar' => 'int', 'length' => 255, 'not null' => TRUE, 'default' => 0));
+  db_change_field('url_alias', 'dst', 'alias', array('varchar' => 'int', 'length' => 255, 'not null' => TRUE, 'default' => 0));

Status:Needs work» Needs review
StatusFileSize
new34.69 KB
Failed: Failed to apply patch.
[ View ]

Here's a re-roll that makes a changes mentioned in #34... I didn't do anything to path_admin_form_submit(), which could still use some cleanup...

Priority:Critical» Normal
Status:Needs review» Needs work

If only we were able to automatically test the upgrade path, huh. ;-)

I went ahead and committed the patch in #35. I'm marking this back as 'code needs work' so we can follow-up with the clean-up (see above).

We should also document these changes in the handbook, and we should consider making a CHANGELOG.txt entry.

Status:Needs work» Needs review
StatusFileSize
new5.12 KB
Failed: 13838 passes, 19 fails, 0 exceptions
[ View ]

You shouldn't say "clean-up" too loud. 8)

The CHANGELOG.txt entry is too much for me. I can't handle that.

Status:Needs review» Needs work

The last submitted patch failed testing.

I believe #606888: Node creation breaks if no path is specified is related to this, so linking it here.

Status:Needs work» Needs review
StatusFileSize
new10.41 KB
Failed: Failed to apply patch.
[ View ]

This patch fixes
1) don't save empty path for term and node
2) if user delete alias on node or term edit page then delete old alias
3) check for emptiness of *string* should use trimmed value
4) factored term-edit page

Suppose this need a lot of attention - some places still no-braid logic

Proposals:
1) path_save - check for empty alias before saving, use drupal_write_record
2) path_load - could return multiple values (criteria => alias without language)
3) unify checks for existence of alias in validate functions
4) optional replace direct queries with path_load (mostly possible)

Priority:Normal» Critical

The more I look into this, the more I realize that this was a badly needed clean-up, and even more importantly, I more and more think that Path module is totally incompatible with translatable fields, because it still relies on $node->language to do its job.

I'm working on this patch now, but I fear that the whole story won't be complete after committing the required changes here. It seems like Path module needs to be a field to work properly with translatable fields.

Assigned:dmitrig01» sun
StatusFileSize
new18.42 KB
Failed: 13252 passes, 23 fails, 5 exceptions
[ View ]

ok.

This is a bit more than a simple fix and clean-up. The old code didn't make sense at all. So what's contained?

  • path_save() properly saves/inserts/updates aliases now and alters the passed in $path by reference to be consistent with other APIs.
  • $path is always an array - regardless of whether by itself, or in $node->path or $term->path.
  • Speaking of, Path module now implements Taxonomy module's API to perform path insertions and updates.
  • The URL alias widget that is displayed in node forms and taxonomy term forms is now an atomic piece that comes with its own #element_validate function. This part could be even more abstracted, but as mentioned before, I don't think that the current implementation can stay as is, because it totally ignores the (content/field) language of the object it is assigned to - hence, this needs to become a Path/URL alias field anyway.
  • The functions have also been slightly re-ordered to make any sense of them... so we now have 1) Path API functions 2) Node API integration functions 3) Taxonomy API integration functions.

All of these steps can be understood as preparation for turning this thingy into a field - which shouldn't be very hard after this patch lands.

I hope the tests will pass -- at least, I've tested this patch manually and everything seems to work.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

I also just realized that those Path API functions really belong into includes/path.inc. Will re-roll with fixed tests and that change in a few minutes.

I also just realized that those Path API functions really belong into includes/path.inc.

Please, no.

Dave Reid: Any real objections beside personal taste? Point is, Path module is only the UI. Drupal core supports URL aliases without Path module being enabled. So the situation is the very same as with menu.inc and Menu module. I'm open to discuss this, but the current implementation/separation doesn't make sense.

We don't support saving menus, that's in menu.module. If you need to do this to get D7 to work, fine. My goal in D8 is to make core work without path.module completely and not just make it a UI module (ala smallcore).

Well, the comparison is wrong. menu_link_build(), menu_link_save(), menu_link_delete() all live in menu.inc, not menu.module.

Yes and Drupal works just fine without path aliases, but not menu links. Like I said, if it works fine, but this entire module needs to be revamped completely for D8. This issue proved it.

Well, sorry, but that's wrong again. Drupal works fine without menu links. You only need the menu router.

Ok. Sun, I know you are working terribly hard recently on D7 work. I didn't want to start a fight, but it just seemed wrong that these belonged in path.inc when path_set_alias lived just fine in path.module up to now. I could have expressed my opinion in a much better and rational way and I apologize. Please continue. I won't try to help in this issue anymore.

+++ modules/path/path.module 17 Oct 2009 19:10:03 -0000
@@ -198,191 +182,203 @@ function path_node_load($nodes, $types)
+    if ($query->execute()->fetchField()) {
+      form_set_error('alias', t('The alias is already in use.'));
+    }

Message was 'The path is already in use.' - I see no reason to change it (Tests check it)

+++ modules/path/path.module 17 Oct 2009 19:10:03 -0000
@@ -198,191 +182,203 @@ function path_node_load($nodes, $types)
+      $path += array(
+        'source' => 'taxonomy/term/' . $term->tid,
+        'language' => '',
+      );

Should be set directly
code>
+++ modules/path/path.module 17 Oct 2009 19:10:03 -0000
@@ -198,191 +182,203 @@ function path_node_load($nodes, $types)
+ $path += array(
+ 'source' => 'node/' . $node->nid,
+ 'language' => isset($node->language) ? $node->language : '',
);

same as above

<?php
     
// Ensure fields for programmatic executions.
     
$path['source'] = 'node/' . $node->nid;
     
$path['language'] = isset($node->language) ? $node->language : '';
?>

StatusFileSize
new25.24 KB
Failed: Failed to apply patch.
[ View ]

This one should pass the tests.

Didn't move Path API functions to make Dave happy.

+++ modules/path/path.module 17 Oct 2009 22:36:21 -0000
@@ -148,40 +164,16 @@ function path_delete($criteria) {
-  if (user_access('create url aliases') || user_access('administer url aliases')) {

Are you sure to remove access checks? If path_node_load attach path to every node, so all hook_node_* would work skipping access check.

+++ modules/path/path.module 17 Oct 2009 22:36:21 -0000
@@ -198,191 +190,211 @@ function path_node_load($nodes, $types)
+ * Implement hook_taxonomy_term_insert().

why not using hook_taxonomy_term_load() ?

This review is powered by Dreditor.

StatusFileSize
new26.75 KB
Failed: Failed to apply patch.
[ View ]

1) Absolutely totally not. The caller is responsible for checking access. Wrong access checks like that were the reason for path_node_delete() not deleting stored URL aliases in case a user who deletes a node does not have URL alias administration permissions. API functions should never deal with access permissions.

2) Thanks for noting that - you just revealed a major slow-down in Drupal! It's totally senseless to implement path_node_load(), because we don't. ever. need. URL. alias. info. for. every. single. node. that. is. loaded. That was even executed for simple node access checks and other stuff where a $node was loaded... Path module only needs this info for its own UI, and every other module is free to use path_load() on its own, but we definitely, absolutely, totally don't want to load URL aliases for every single loaded node without having *any* use for that info. :)

StatusFileSize
new25.88 KB
Failed: Failed to apply patch.
[ View ]

Looks great, except taxonomy (tested by hands - suppose we need tests)

I think we can extend taxonomy.test with path only thing we need - save term with alias and check path_load

Attached patch with fixed texonomy

+++ modules/path/path.module 18 Oct 2009 00:01:01 -0000
@@ -303,86 +310,82 @@ function path_form_alter(&$form, $form_s
+      // Ensure fields for programmatic executions.
+      $path += array(
+        'source' => 'taxonomy/term/' . $term->tid,
+        'language' => '',
+      );

does not work!

+++ modules/path/path.module 18 Oct 2009 00:01:01 -0000
@@ -303,86 +310,82 @@ function path_form_alter(&$form, $form_s
+      // Ensure fields for programmatic executions.
+      $path += array(
+        'source' => 'taxonomy/term/' . $term->tid,
+        'language' => '',
+      );
+      path_save($path);

You should use

<?php
     
// Ensure fields for programmatic executions.
     
$path['source'] = 'taxonomy/term/' . $term->tid;
     
$path['language'] = '';
?>

UPD: path source point to term^^

About path_node_load() should be documented because contrib can rely on this

StatusFileSize
new29.65 KB
Failed: Failed to apply patch.
[ View ]

Now including taxonomy term alias test.

We also want to add a test that prevents us from breaking this whole thing again.

+++ modules/path/path.test 18 Oct 2009 20:47:40 -0000
@@ -157,6 +154,68 @@ class PathTestCase extends DrupalWebTest
+    // Change the term's URL alias.
+    $tid = db_query("SELECT tid FROM {taxonomy_term_data} WHERE name = :name", array(':name' => $edit['name']))->fetchField();
+    $edit2 = array();
+    $edit2['path[alias]'] = $this->randomName();
+    $this->drupalPost('taxonomy/term/' . $tid . '/edit', $edit2, t('Save'));

<?php
$path
= path_load(array('source' => 'taxonomy/term/ . $tid));
$this->assertTrue($path,
$this->assertEqual($path['
alias'], $edit['path[alias]'],
?>

This check new API function and empty text and path assigning to term

Suppose this is enought

This review is powered by Dreditor.

No. We want to test the actual interface interaction, because we are implementing integration functions for taxonomy terms.

What you are proposing is a CRUD test. A CRUD test would look entirely different.

Status:Needs review» Reviewed & tested by the community

+++ modules/path/path.test 18 Oct 2009 20:47:40 -0000
@@ -157,6 +154,68 @@ class PathTestCase extends DrupalWebTest
+    // Confirm that the alias works.
+    $this->drupalGet($edit['path[alias]']);
+    $this->assertText($edit['description'], 'Term can be accessed on URL alias.');
+

This is a check for existence and not empty

+++ modules/path/path.test 18 Oct 2009 20:47:40 -0000
@@ -157,6 +154,68 @@ class PathTestCase extends DrupalWebTest
+    // Change the term's URL alias.
+    $tid = db_query("SELECT tid FROM {taxonomy_term_data} WHERE name = :name", array(':name' => $edit['name']))->fetchField();
+    $edit2 = array();

This is another check for emptyness

+++ modules/path/path.test 18 Oct 2009 20:47:40 -0000
@@ -203,11 +259,11 @@ class PathLanguageTestCase extends Drupa
     // Edit the node to set language and path.
     $edit = array();
     $edit['language'] = 'en';
-    $edit['path'] = $this->randomName();
+    $edit['path[alias]'] = $this->randomName();
     $this->drupalPost('node/' . $english_node->nid . '/edit', $edit, t('Save'));

same test for node

Suppose it's time for RTBC

I reviewed this patch and it looks good. Only one thing stood out:

+++ modules/path/path.module 18 Oct 2009 19:30:26 -0000
@@ -148,51 +164,96 @@ function path_delete($criteria) {
+    // Node language (Translation module) needs special care. Since the language
+    // of the URL alias depends on the node language and the node language can
+    // be switched right within the same form, we need to conditionally overload
+    // the originally assigned URL alias language.
+    if (isset($form_state['values']['language'])) {
+      form_set_value($element['language'], $form_state['values']['language'], $form_state);
+    }

I don't understand the reference to Node language or Translation module here.

StatusFileSize
new29.8 KB
Failed: Failed to apply patch.
[ View ]

That's very likely, because Locale module doesn't make any sense at all currently.

1) You're right, it's not Translation module, but Locale module that needs special treatment, because it's not even Translation module that adds the language selector.

2) It is utterly wrong that Locale module assigns a language to content. Locale is about UI language, not content language.

3) But, anyway - Locale module adds a language selector to the node form to assign a language to the posted node content. This language must be used for generating a URL alias. And since that language can be altered by the user within the same form in which the URL alias widget lives, and since neither of both widgets know anything about the real content language, we have to take over the selected language from Locale's language selector.

New docs:

    // Node language (Locale module) needs special care. Since the language of
    // the URL alias depends on the node language, and the node language can be
    // switched right within the same form, we need to conditionally overload
    // the originally assigned URL alias language.
    // @todo Remove this after converting Path module to a field, and, after
    //   stopping Locale module from abusing the content language system.
    if (isset($form_state['values']['language'])) {
      form_set_value($element['language'], $form_state['values']['language'], $form_state);
    }

Status:Reviewed & tested by the community» Fixed

Patch contains some fixes so committed to CVS HEAD.

I agree that the new path CRUD belongs in path.inc.

Status:Fixed» Needs review
StatusFileSize
new5.3 KB
Passed: 14394 passes, 0 fails, 0 exceptions
[ View ]

I still agree with that as well.

Hm, so path.inc should hold double api? How it possible to document?

No, there is only one Path API, and those CRUD functions belong to it.

The Path module is just a UI module to configure/administer URL aliases. Just like Menu module is one of many possible UIs to administer your menus and menu links. Or just like Upload/File modules are some possible UIs to upload files.

path_load seems like http://api.drupal.org/api/function/drupal_get_path_alias/7

But las one is not using it

This patch makes a lot of sense to me also.

Re #49, even if you want to make URL alias lookups completely optional in Drupal 8, I still think the patch here will make sense. The way to do that would be to turn path.inc from an include file into a module, so having proper separation between it and path.module (the UI) is a prerequisite for that.

Re #72, drupal_get_path_alias() is a highly-optimized function for a very specific purpose, so I'm not sure we want to try to merge them. That said, it may make sense to document the difference between them more carefully? Also, with this patch, the namespace in path.inc is a bit funny (some are drupal_, some are path_)...

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-DX (Developer Experience), -API clean-up, -D7 API clean-up

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