Problem/Motivation

\Drupal\Core\Entity\Query\Sql\Tables::addField() needlessly repeats some code in an if/else branch.

Proposed resolution

Move the repeated code out of the condition.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#2 2956411.patch3.05 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2956411.patch3.05 KB

And a patch.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue tags: +Quickfix
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

I stared at this a while and tried to find functional differences.

The only thing I came across was: $column having a truthy value is now no longer guarded by $table_mapping->requiresDedicatedTableStorage($field_storage). It also has a value of NULL for the rest of the method instead of not being set at all. $column is only used within blocks of code that is guarded by a truthy value of $field_storage, so this slight shuffle around produces no functional change whatsoever.

RTBC based on the before and after being equivalent, if anyone thinks this needs a review from someone familiar with the subsystem please set back to NR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 44b9b25 and pushed to 8.6.x. Thanks!

  • alexpott committed 44b9b25 on 8.6.x 
    Issue #2956411 by amateescu: Simplify \Drupal\Core\Entity\Query\Sql\...

Status: Fixed » Closed (fixed)

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