See #1328180: Minor error when upgrading nodes programmaticly.. We want people to be able to define $node->path['pathauto'], but this causes a PHP notice because path_node_insert/update and path_taxonomy_term_insert/update only check if isset($item->path) and not isset($item->path['alias']) which seems more reasonable.

CommentFileSizeAuthor
#10 1576552-path-notice-9.patch1015 bytesjcisio
#6 drupal-path-php-notice-1576552-06.patch861 bytesjhedstrom
#1 drupal-path-prevent_notices-1576552-1.patch920 bytesmarkie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markie’s picture

markie CreditAttribution: markie commented
Status: Active » Needs review
FileSize
drupal-path-prevent_notices-1576552-1.patch920 bytes

pretty basic patch attached.

jhedstrom queued 1: drupal-path-prevent_notices-1576552-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal-path-prevent_notices-1576552-1.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes
Issue tags: +needs-reroll
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 8.0.x-dev » 7.x-dev

I don't think this applies to 8.x anymore.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs work » Needs review
Issue tags: -
FileSize
drupal-path-php-notice-1576552-06.patch861 bytes

Re-rolled for D7.

SpadXIII’s picture

SpadXIII CreditAttribution: SpadXIII commented
Status: Needs review » Reviewed & tested by the community

Looks like a nice and simple patch that works like a charm!

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Needs review

The path_node_insert() change looks fine, but I'm not sure about the way the patch does it in path_node_update() since that function contains this code further down:

    // Delete old alias if user erased it.
    if (!empty($path['pid']) && empty($path['alias'])) {
      path_delete($path['pid']);
    }

So if you pass a 'pid' without an 'alias', currently it will run path_delete() (albeit with a PHP notice), but with the patch, path_delete() will be skipped entirely. I'm not sure what the practical consequences of that would be, but do we really want to change that behavior as a side effect of fixing the PHP notice...?

jcisio’s picture

jcisio CreditAttribution: jcisio at Axess Open Web Services commented

I don't know we can have !empty($path['pid']) with $path['alias'] unset. BTW attached patch to keep the current behavior and to fix the notices.

jcisio’s picture

jcisio CreditAttribution: jcisio at Axess Open Web Services commented
FileSize
1576552-path-notice-9.patch1015 bytes
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yup, that looks good now and seems to work correctly.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed e719d5c on 7.x 
    Issue #1576552 by jhedstrom, jcisio, markie: Prevent PHP notices in...

Status: Fixed » Closed (fixed)

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