This is coming in response to #1337500: Restrict user to only set path on assigned domains, which needs domain_path_node_insert() to know an existing path's dpid so that drupal_write_record() can update only that record, rather than destroying all values and writing them new each time. This would be more congruent with path.module's handling of the same in core.

Unfortunately, I think this means the structure of the return array from domain_path_paths() needs to change. Instead of the following

$paths[$key][$nid][$domain_id] = 'alias as a string';

That value has to be an associative array or object:

$paths[$key][$nid][$domain_id] = array(
  'dpid' => $dpid,
  'source' => $source,
  'alias' => $alias,
);

This also means corresponding changes to deal with the new structure wherever domain_path_paths() is called.

CommentFileSizeAuthor
#20 1425292-20-domain_path-field_access.patch12.75 KBgrndlvl
#14 1425292-14-domain_path-update_existing_records.patch6.19 KBgrndlvl
#10 domain_path-update_exisiting_records-1425292-10.patch6.16 KBgrndlvl
#9 domain_path-update_exisiting_records-1425292-9.patch6.1 KBgrndlvl
#1 domain_path-1425292.patch2.55 KBLes Lim
#2 domain_path-1425292-2.patch5.68 KBLes Lim
#2 interdiff-1425292-1-2.diff3.63 KBLes Lim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
FileSize
domain_path-1425292.patch2.55 KB

Here's a patch altering domain_path_paths() to return the full row from {domain_path} including dpid. Not actually using the API change for anything yet here.

EDIT: Note that this requires a cache clear.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Status: Active » Needs review
FileSize
interdiff-1425292-1-2.diff3.63 KB
domain_path-1425292-2.patch5.68 KB

This one uses the API change to act on individual records within {domain_path} in domain_path_node_insert(). Existing table records will be updated rather than destroyed and created new.

By the way, the cache must be cleared after patching with either of these. The module will need an update function if this makes it in.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Title: domain_path_paths() should return 'dpid' values » domain_path_node_insert() should update existing records rather than deleting/recreating them
Issue tags: +API change
Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Issue summary: View changes

minor clarifications

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Makes sense.

If you have any interest in co-maintaining this module, let me know.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Status: Needs review » Needs work

No longer applies to 7.x-1.x since domain_path_paths was replaced in favor of domain_path_lookup_path.

I will be looking into this probably later today. We probably need a domain_path_load similar to path_load to accomplish this.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented

I like the parallelism with naming the function domain_path_load(), but it should probably be domain_path_path_load() to avoid confusion with hook_load().

I missed that there was new development in 7.x-1.x. Are you keeping snapshot releases off intentionally?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Had been, but I'll activate them now.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Snapshots are active. https://drupal.org/node/1027556

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Status: Needs work » Needs review
FileSize
domain_path-update_exisiting_records-1425292-9.patch6.1 KB

Re domain_path_path_load(): Went ahead and used domain_path_load() anyways since it would not interfere with nodes hook_load() as we don't implement a node, and it is just cleaner.

hook_load();
This hook is invoked only on the module that defines the node's content type (use hook_node_load() to respond to all node loads).

Some new additions:
- domain_path_load
- domain_path_save
- domain_path_save_paths
- domain_path_delete

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
FileSize
domain_path-update_exisiting_records-1425292-10.patch6.16 KB

- Removing the delete out of domain_path_save and place back into the node submission.
- Automatically detecting the current domain id for domain_path_load when a domain id was not sepecified

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented

Bueller?

Thinking about it more I will probably go ahead and change the load function's name, but that's simple enough I can do it after we get an initial review.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Not crazy about the $conditions parameter's mutability. The rest seems fine.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Assigned: Unassigned » grndlvl
Status: Needs review » Needs work

Doesn't work against latest dev.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
FileSize
1425292-14-domain_path-update_existing_records.patch6.19 KB

Last wasn't an error it was an issue w/ GlobalRedirect which we are currently looking into.

Re mutability:
This is because of the various ways a domain is looked up. This happens in Drupal's http://api.drupal.org/api/drupal/includes!path.inc/function/path_load/7. So I am inclined to want to leave it.

Unless I hear a response I will assume this is ok and commit.

Patch changes:
- rename domain_path_load to domain_path_path_load
- re-roll against dev

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Status: Needs work » Needs review
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs review » Needs work

return $foo Really? That needs a real variable name. Otherwise I have no objections.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented

lol looks like some test code made in the patch :) yes I will def. remove that

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Status: Needs work » Closed (fixed)

Committed http://drupalcode.org/project/domain_path.git/commit/7ae9ace8002a83b881f...

Thanks guys.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Status: Closed (fixed) » Needs work
Issue tags: +Needs tests

Actually we should add tests for this.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Assigned: grndlvl » Unassigned
Status: Needs work » Needs review
FileSize
1425292-20-domain_path-field_access.patch12.75 KB

Requires patch from #1337500: Restrict user to only set path on assigned domains at comment 11.

grndlvl’s picture

grndlvl CreditAttribution: grndlvl commented
Issue summary: View changes

Specify which function needs the API change