Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#69 | drupal.path-inc-api.patch | 5.3 KB | sun |
#66 | drupal.path-api-major-fixes.66.patch | 29.8 KB | sun |
#60 | drupal.path-api-major-fixes.60.patch | 29.65 KB | sun |
#58 | 332333_path-api-major-fixes.patch | 25.88 KB | andypost |
#57 | drupal.path-api-major-fixes.57.patch | 26.75 KB | sun |
Comments
Comment #1
chx CreditAttribution: chx commentedDo, or do not. There is no 'try.' If you want to rename dst and src then rename them throughout core.
Comment #2
Dave ReidComment #3
dropcube CreditAttribution: dropcube commentedtagging
Comment #4
catchComment #5
Dave ReidI 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.
Comment #6
dmitrig01 CreditAttribution: dmitrig01 commentedThis patch
- Adds real api functions
- Converts src and dst to source and alias because we don't abrv8
Comment #7
dmitrig01 CreditAttribution: dmitrig01 commentedand needs review (and testingbot's input)
Comment #8
dmitrig01 CreditAttribution: dmitrig01 commentedOops
Comment #10
dmitrig01 CreditAttribution: dmitrig01 commentedThis patch should pass almost all tests except for two fails which I'm having an impossible time tracking down.
Comment #12
dmitrig01 CreditAttribution: dmitrig01 commentedmuch improved
Comment #13
dmitrig01 CreditAttribution: dmitrig01 commentedAlso, this patch gets rid of the beast that is path_set_alias
Comment #15
Dave ReidWoot! Keep 'em coming dmitrig01! I'll review once it gets a green from the testbot!
Comment #16
dmitrig01 CreditAttribution: dmitrig01 commentedPerhaps without a parse error this time.
Comment #18
grendzy CreditAttribution: grendzy commentedThis seems to resolve the test:
Comment #19
dmitrig01 CreditAttribution: dmitrig01 commentedGrendzy's solution in patch form
Comment #21
dmitrig01 CreditAttribution: dmitrig01 commentedwith less (no?) exceptions
Comment #22
dmitrig01 CreditAttribution: dmitrig01 commentedComment #23
Dries CreditAttribution: Dries commentedAt a high-level, this looks like a nice clean-up. Can't do an in-depth review right now though.
Comment #24
dmitrig01 CreditAttribution: dmitrig01 commentedComment #25
dmitrig01 CreditAttribution: dmitrig01 commentedAdding appropriate tag
Comment #26
catchThis looks great. Tests would fail dismally if this introduced bugs, path module is pretty well tested. Just found this:
Missing one line summary.
I'm on crack. Are you, too?
Comment #27
dmitrig01 CreditAttribution: dmitrig01 commentedComment #28
sunFirst 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:
You could avoid that if #594650: Provide central $form_state['values'] clearance was in...
Just mentioning.
As of today - YAY! - #582456: Make confirm_form() respect query string destination
You can drop that $_GET munging. :)
Missing blank line.
1) Missing PHPDoc summary.
2) Wrong multiline comment syntax.
Duplicate newline.
This review is powered by Dreditor.
Comment #30
dmitrig01 CreditAttribution: dmitrig01 commentedImplemented sun's suggestoins
Comment #31
sunI 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.
Comment #32
Dries CreditAttribution: Dries commentedI committed #594650: Provide central $form_state['values'] clearance so path_admin_form_submit() could be cleaned up a bit.
Comment #33
Dries CreditAttribution: Dries commentedAlso, 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
.Comment #34
Dave ReidYeah, pretty sure we want to keep those as varchars and not restrict ourselves to integer paths. :)
Comment #35
a25i CreditAttribution: a25i commentedHere'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...
Comment #36
Dries CreditAttribution: Dries commentedIf 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.
Comment #37
sunYou shouldn't say "clean-up" too loud. 8)
Comment #38
sunThe CHANGELOG.txt entry is too much for me. I can't handle that.
Comment #40
ksenzeeI believe #606888: Node creation breaks if no path is specified is related to this, so linking it here.
Comment #41
andypostThis 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
Comment #42
andypostProposals:
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)
Comment #43
sunThe 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.
Comment #44
sunok.
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.
Comment #46
sunI 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.
Comment #47
Dave ReidPlease, no.
Comment #48
sunDave 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.
Comment #49
Dave ReidWe 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).
Comment #50
sunWell, the comparison is wrong. menu_link_build(), menu_link_save(), menu_link_delete() all live in menu.inc, not menu.module.
Comment #51
Dave ReidYes 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.
Comment #52
sunWell, sorry, but that's wrong again. Drupal works fine without menu links. You only need the menu router.
Comment #53
Dave ReidOk. 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.
Comment #54
andypostMessage was 'The path is already in use.' - I see no reason to change it (Tests check it)
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
Comment #55
sunThis one should pass the tests.
Didn't move Path API functions to make Dave happy.
Comment #56
andypostAre you sure to remove access checks? If path_node_load attach path to every node, so all hook_node_* would work skipping access check.
why not using hook_taxonomy_term_load() ?
This review is powered by Dreditor.
Comment #57
sun1) 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. :)
Comment #58
andypostLooks 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
does not work!
You should use
UPD: path source point to term^^
Comment #59
andypostAbout path_node_load() should be documented because contrib can rely on this
Comment #60
sunNow including taxonomy term alias test.
Comment #61
sunWe also want to add a test that prevents us from breaking this whole thing again.
Comment #62
andypostThis check new API function and empty text and path assigning to term
Suppose this is enought
This review is powered by Dreditor.
Comment #63
sunNo. 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.
Comment #64
andypostThis is a check for existence and not empty
This is another check for emptyness
same test for node
Suppose it's time for RTBC
Comment #65
Dries CreditAttribution: Dries commentedI reviewed this patch and it looks good. Only one thing stood out:
I don't understand the reference to Node language or Translation module here.
Comment #66
sunThat'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:
Comment #67
Dries CreditAttribution: Dries commentedPatch contains some fixes so committed to CVS HEAD.
Comment #68
moshe weitzman CreditAttribution: moshe weitzman commentedI agree that the new path CRUD belongs in path.inc.
Comment #69
sunI still agree with that as well.
Comment #70
andypostHm, so path.inc should hold double api? How it possible to document?
Comment #71
sunNo, 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.
Comment #72
andypostpath_load seems like http://api.drupal.org/api/function/drupal_get_path_alias/7
But las one is not using it
Comment #73
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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_)...
Comment #74
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #75
catchSee also: #612920: {url_alias}.source required for upgrade