Problem

  1. Merge::key() accepts an associative array possibly containing multiple values.
  2. Merge::fields(), Merge::values(), etc are all plural.
  3. But yet, Merge::key() is singular.

Proposed solution

  1. Rename the method into Merge::keys(), but provide a backwards-compatible alias Merge::key().
CommentFileSizeAuthor
#9 interdiff.txt1.01 KBsun
#9 merge.keys_.9.patch20.18 KBsun
#6 interdiff.txt20.21 KBsun
#6 merge.keys_.6.patch20.31 KBsun
#4 drupal8.db-merge-key.4.patch1.03 KBsun
drupal8.db-merge-key.0.patch1.06 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Rename Database\Query\Merge::key() to keys(), or at least provide an alias » Rename Database\Query\Merge::key() to keys(), retain a BC alias for key()
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

simple enough.

Crell’s picture

Crell commented

The typical case for a merge query is a single-value key; multi-value keys are not common IME. That's why it was called key() originally. If we do this, I would not mark key() deprecated. Just call it an alias and be done with it.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.db-merge-key.4.patch1.03 KB

Happy to do so :-)

That said, given the explanation for why the method is using a singular name (thanks!), I wondered whether this split here wouldn't actually allow us to give key() a signature that corresponds to its intended use-case?

Like this:

  public function key($field, $value) {
    $this->keys(array($field => $value));
    return $this;
  }

Might be worth as a follow-up issue?

Crell’s picture

Crell commented
Status: Reviewed & tested by the community » Needs work

Good idea. Let's go ahead and do that now. So keys() takes a single associative array, key() takes a singluar key and value. That's more descriptive and non-redundant. And I think not even an API change. :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new merge.keys_.6.patch20.31 KB
new interdiff.txt20.21 KB
  1. Changed signature of Merge::key(), but retaining a BC shim.
  2. Converted all Merge queries to either use keys(array) or key(field, value).

The last submitted patch, 4: drupal8.db-merge-key.4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: merge.keys_.6.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new merge.keys_.9.patch20.18 KB
new interdiff.txt1.01 KB

Reverted change to invocation order in Cache\DatabaseBackend.

This should come back green again now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Draft change notice: https://drupal.org/node/2205327

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

9: merge.keys_.9.patch queued for re-testing.

Crell’s picture

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

Done and done. Thanks, sun.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Nice little API improvement. Committed to 8.x.

Status: Fixed » Closed (fixed)

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