Contrib.social

UpdateQuery::arguments() does not return all values.

Open on Drupal.org →
Created on 28 May 2013, over 11 years ago
Updated 15 May 2024, 7 months ago

With reference to #1222150: dpq() only works for SELECT and INSERT queries → (a devel module issue), an outstanding issue with the output of its dpq() function on db_update() queries is that the field placeholders are still rendered as the placeholder name, not the value.

e.g. dpq(db_update('node')->fields(array('uid' => 1))->condition('nid', 50)); produces:

UPDATE {node} SET uid=:db_update_placeholder_0
WHERE  (nid = '50')

With arguments() returning:

Array
(
    [:db_condition_placeholder_0] => 50
)

This is because UpdateQuery::arguments() returns the db_condition_placeholder values, but does not return the db_update_placeholder values.

The reason the query still works is that UpdateQuery::execute() deals with these field placeholders separately; however it access them directly, and no public API analogous to arguments() is provided, so the likes of devel's dpq() have no way of obtaining them.

Now the docstring for arguments() is "Gets a complete list of all values to insert into the prepared statement.", and that's obviously not the case here, but given how the execute method works, I can't tell whether it's the docstring or the code which is incorrect. I can't actually see a reason not to include the field placeholders, though; because...

Unless I'm mis-reading my grep results, I don't think anything in core calls UpdateQuery::arguments() at all? SelectQuery::arguments() is certainly used, and $condition->arguments() is called directly in lots of places, but nothing in my code base save for devel's dpq() seems like it would ever to call this method on an update query.

Experimentally (using code from UpdateQuery::__toString()), if instead of only returning $this->condition->arguments(), I modify UpdateQuery::arguments() like so:

  /**
   * Implements QueryConditionInterface::arguments().
   */
  public function arguments() {
    $args = $this->condition->arguments();

    $fields = $this->fields;
    $max_placeholder = 0;
    foreach ($fields as $field => $value) {
      $args[':db_update_placeholder_' . ($max_placeholder++)] = $value;
    }
    return $args;
  }

Then arguments() returns:

Array
(
    [:db_condition_placeholder_0] => 50
    [:db_update_placeholder_0] => 1
)

and dpq() gives me:

UPDATE {node} SET uid='1'
WHERE  (nid = '50')

So that seems like a viable solution?

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Database  →

Last updated 2 days ago

  • Maintained by
  • 🇳🇱Netherlands @daffie
Created by

🇳🇿New Zealand jweowu

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !8080implements arguments function in update → (Open) created by sukr_s
  • Pipeline finished with Failed
    7 months ago
    Total: 189s
    #172981
  • Pipeline finished with Failed
    7 months ago
    Total: 1080s
    #172991
  • Pipeline finished with Canceled
    7 months ago
    Total: 9s
    #173008
  • Pipeline finished with Success
    7 months ago
    Total: 615s
    #173009
  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇮🇳India sukr_s

    - Moved the patch to MR
    - Update the test for comment #30

  • Status changed to Needs work 7 months ago
  • Comment 7 months ago →
    🇺🇸United States smustgrave

    Left a small comment

    Issue summary should follow standard issue template so tagged for such

    Thanks.

  • Pipeline finished with Failed
    7 months ago
    Total: 958s
    #184664
  • Pipeline finished with Success
    7 months ago
    Total: 588s
    #184682
  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇮🇳India sukr_s

    IS updated and review comment resolved.

  • Status changed to RTBC 7 months ago
  • Comment 7 months ago →
    🇺🇸United States smustgrave

    Feedback appears to be resolved.

  • Status changed to Needs work 7 months ago
  • Comment 7 months ago →
    🇬🇧United Kingdom longwave UK

    Added some comments to the MR.

  • Pipeline finished with Failed
    6 months ago
    Total: 531s
    #195273
  • Pipeline finished with Success
    6 months ago
    Total: 512s
    #195293
  • Status changed to Needs review 6 months ago
  • Comment 6 months ago →
    🇮🇳India sukr_s

    MR comments resolved.

  • Status changed to RTBC 6 months ago
  • Comment 6 months ago →
    🇺🇸United States smustgrave

    Refactoring suggestions appear to not have broken anything.

    Left a comment to confirm that @inheritdoc is inherited, least according to phpstorm.

  • First commit to issue fork.
  • Status changed to Needs review 6 months ago
  • Comment 6 months ago →
    🇳🇿New Zealand quietone

    I read the IS, comments and the MR. There is discussion in #6 and #7 about refactoring that I think was done in response to longwave's suggestion in this commit. I didn't find any other unanswered questions.

    I made some suggestions to comments and the test, which I have then applied. I think they should be reviewed before this goes back to RTBC.

  • Status changed to RTBC 6 months ago
  • Comment 6 months ago →
    🇺🇸United States smustgrave

    This was posted in #needs-review-queue-initative in slack so came to take a look.

    Reviewing the comment changes in this commit https://git.drupalcode.org/project/drupal/-/merge_requests/8080/diffs?co...

    Returns the query arguments with placeholders mapped to their values.

    was the only one I wasn't sure was needed but saying it out loud I believe it's fine.

    The others are definitely improvements.

    Believe this one is good.

  • Status changed to Needs work 5 months ago
  • Comment 5 months ago →
    🇬🇧United Kingdom longwave UK

    There is a mismatch in the return value in the new method, and it made me realise the new method is doing two jobs; should we split it into two methods?

  • Pipeline finished with Failed
    5 months ago
    Total: 458s
    #234506
  • Pipeline finished with Success
    5 months ago
    #234540
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇮🇳India sukr_s

    I've updated the comment. Splitting it into two methods will not help since the $args should not have expression placeholders. so the first loop will have to be repeated again or additional checks done to exclude the expression placeholders.

  • Status changed to RTBC 4 months ago
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Believe feedback has been addressed and inheritdoc appears to be getting picked up now.

  • Comment 3 months ago →
    🇬🇧United Kingdom longwave UK

    Not sure about committing this to a patch release just in case someone is relying on the existing output for some reason. Backported to 10.4.x to keep the API in sync though.

    Committed and pushed ea261a9d1a5 to 11.x and ffecc0bccfc to 10.4.x. Thanks!

  • Comment 3 months ago →
    System Message
    • longwave → committed ffecc0bc on 10.4.x 
      Issue #2005626 by sukr_s, gold, jhedstrom, neelam_wadhwani, neslee canil...
  • Comment 3 months ago →
    System Message
    • longwave → committed ea261a9d on 11.x 
      Issue #2005626 by sukr_s, gold, jhedstrom, neelam_wadhwani, neslee canil...
  • Status changed to Fixed 2 months ago
  • Comment 2 months ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024