Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

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.

Dave Reid’s picture

Priority: Critical » Normal
dropcube’s picture

tagging

catch’s picture

Version: 7.x-dev » 8.x-dev
Dave Reid’s picture

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.

dmitrig01’s picture

FileSize
31.3 KB

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

dmitrig01’s picture

Status: Needs work » Needs review

and needs review (and testingbot's input)

dmitrig01’s picture

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

Oops

Status: Needs review » Needs work

The last submitted patch failed testing.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
33.74 KB

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.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
34.21 KB

much improved

dmitrig01’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Status: Needs work » Needs review

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

dmitrig01’s picture

FileSize
35.08 KB

Perhaps without a parse error this time.

Status: Needs review » Needs work

The last submitted patch failed testing.

grendzy’s picture

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;
+      }
     }
dmitrig01’s picture

Status: Needs work » Needs review
FileSize
35.16 KB

Grendzy's solution in patch form

Status: Needs review » Needs work

The last submitted patch failed testing.

dmitrig01’s picture

FileSize
35.31 KB

with less (no?) exceptions

dmitrig01’s picture

Status: Needs work » Needs review
Dries’s picture

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

dmitrig01’s picture

Assigned: Dave Reid » dmitrig01
dmitrig01’s picture

Issue tags: +D7 API clean-up

Adding appropriate tag

catch’s picture

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?

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
35.31 KB
sun’s picture

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.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
36.47 KB

Implemented sun's suggestoins

sun’s picture

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.

Dries’s picture

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

Dries’s picture

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.

Dave Reid’s picture

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));
a25i’s picture

Status: Needs work » Needs review
FileSize
34.69 KB

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...

Dries’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
5.12 KB

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

sun’s picture

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.

ksenzee’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
10.41 KB

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

andypost’s picture

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)

sun’s picture

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.

sun’s picture

Assigned: dmitrig01 » sun
FileSize
18.42 KB

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.

sun’s picture

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.

Dave Reid’s picture

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

Please, no.

sun’s picture

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.

Dave Reid’s picture

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).

sun’s picture

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

Dave Reid’s picture

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.

sun’s picture

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

Dave Reid’s picture

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.

andypost’s picture

+++ 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

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

This one should pass the tests.

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

andypost’s picture

+++ 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.

sun’s picture

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. :)

andypost’s picture

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

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

UPD: path source point to term^^

andypost’s picture

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

sun’s picture

Now including taxonomy term alias test.

sun’s picture

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

andypost’s picture

+++ 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'));
$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.

sun’s picture

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.

andypost’s picture

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

Dries’s picture

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.

sun’s picture

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);
    }
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Patch contains some fixes so committed to CVS HEAD.

moshe weitzman’s picture

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

sun’s picture

Status: Fixed » Needs review
FileSize
5.3 KB

I still agree with that as well.

andypost’s picture

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

sun’s picture

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.

andypost’s picture

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

But las one is not using it

David_Rothstein’s picture

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_)...

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

catch’s picture

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.