Add a real API to path.module
dmitrig01 - November 10, 2008 - 06:24
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | path.module |
| Category: | task |
| Priority: | critical |
| Assigned: | sun |
| Status: | closed |
| Issue tags: | API clean-up, D7 API clean-up, DX (Developer Experience) |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| path_module_needs_a_real_api.patch | 18.13 KB | Idle | Failed: Failed to apply patch. | View details |
#1
Do, or do not. There is no 'try.' If you want to rename dst and src then rename them throughout core.
#2
#3
tagging
#4
#5
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.
#6
This patch
- Adds real api functions
- Converts src and dst to source and alias because we don't abrv8
#7
and needs review (and testingbot's input)
#8
Oops
#9
The last submitted patch failed testing.
#10
This patch should pass almost all tests except for two fails which I'm having an impossible time tracking down.
#11
The last submitted patch failed testing.
#12
much improved
#13
Also, this patch gets rid of the beast that is path_set_alias
#14
The last submitted patch failed testing.
#15
Woot! Keep 'em coming dmitrig01! I'll review once it gets a green from the testbot!
#16
Perhaps without a parse error this time.
#17
The last submitted patch failed testing.
#18
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;
+ }
}
#19
Grendzy's solution in patch form
#20
The last submitted patch failed testing.
#21
with less (no?) exceptions
#22
#23
At a high-level, this looks like a nice clean-up. Can't do an in-depth review right now though.
#24
#25
Adding appropriate tag
#26
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?
#27
#28
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.
#29
The last submitted patch failed testing.
#30
Implemented sun's suggestoins
#31
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.
#32
I committed #594650: Provide central $form_state['values'] clearance so path_admin_form_submit() could be cleaned up a bit.
#33
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.#34
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));
#35
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...
#36
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.
#37
You shouldn't say "clean-up" too loud. 8)
#38
The CHANGELOG.txt entry is too much for me. I can't handle that.
#39
The last submitted patch failed testing.
#40
I believe #606888: Node creation breaks if no path is specified is related to this, so linking it here.
#41
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
#42
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)
#43
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.
#44
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?
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.
#45
The last submitted patch failed testing.
#46
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.
#47
Please, no.
#48
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.
#49
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).
#50
Well, the comparison is wrong. menu_link_build(), menu_link_save(), menu_link_delete() all live in menu.inc, not menu.module.
#51
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.
#52
Well, sorry, but that's wrong again. Drupal works fine without menu links. You only need the menu router.
#53
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.
#54
+++ 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 : '';
?>
#55
This one should pass the tests.
Didn't move Path API functions to make Dave happy.
#56
+++ 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.
#57
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. :)
#58
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^^
#59
About path_node_load() should be documented because contrib can rely on this
#60
Now including taxonomy term alias test.
#61
We also want to add a test that prevents us from breaking this whole thing again.
#62
+++ 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.
#63
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.
#64
+++ 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
#65
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.
#66
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);
}
#67
Patch contains some fixes so committed to CVS HEAD.
#68
I agree that the new path CRUD belongs in path.inc.
#69
I still agree with that as well.
#70
Hm, so path.inc should hold double api? How it possible to document?
#71
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.
#72
path_load seems like http://api.drupal.org/api/function/drupal_get_path_alias/7
But las one is not using it
#73
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_)...
#74
Committed to CVS HEAD. Thanks!
#75
See also: #612920: {url_alias}.source required for upgrade
#76
Automatically closed -- issue fixed for 2 weeks with no activity.