Problem/Motivation

Currently we test for empty() and return the current bundle in FeaturesAssigner::getBundle():

  public function getBundle($name = NULL) {
    if (empty($name)) {
      return $this->currentBundle;
    }
    elseif (isset($this->bundles[$name])) {
      return $this->bundles[$name];
    }
    return NULL;
  }

In many places we use an empty string as the ID (machine name) of the default bundle. Example from FeaturesAssigner::findBundle():

    if (!isset($bundle)) {
      // Return the default bundle.
      return $this->getBundle('');
    }

An empty string $name argument to ::getBundle() will therefore return the current bundle rather than the default one.

Proposed resolution

  • Change the DEFAULTBUNDLE constant value from '_default_' to '' so that it matches actual usage.
  • In FeaturesAssigner::getBundle(), test for isset() or is_null() rather than empty().

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 features-bundle-2615634-7.patch893 bytesnedjo
#5 features-bundle-2615634-5.patch450 bytesnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Related issues: +#2569149: Use ConfigEntity for features bundles

Adding a related issue: #2569149: Use ConfigEntity for features bundles

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

In many places we use an empty string as the ID (machine name) of the default bundle.

What about using the constant in those places as well (see https://3v4l.org/FMWpe), so that getBundle would look like this: 

  public function getBundle($name = \Drupal\features\FeaturesAssigner::DEFAULTBUNDLE) {
    if ($name === \Drupal\features\FeaturesAssigner::DEFAULTBUNDLE) {
      return $this->currentBundle;
    }
    elseif (isset($this->bundles[$name])) {
      return $this->bundles[$name];
    }
    // @todo random thought: Is this state a valid one?
    return NULL;
  }
nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo at Chocolate Lily commented
Status: Active » Postponed

Most of the needed work has been rolled into #2569149: Use ConfigEntity for features bundles.

However, in FeaturesAssigner::getBundle() we should still change

    if (empty($name)) {
      return $this->currentBundle;
    }

to

    if (!isset($name)) {
      return $this->currentBundle;
    }
nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo at Chocolate Lily commented
Priority: Normal » Minor
Status: Postponed » Needs review
FileSize
features-bundle-2615634-5.patch450 bytes

This is now minor cleanup.

Status: Needs review » Needs work

The last submitted patch, 5: features-bundle-2615634-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo at Chocolate Lily commented
Status: Needs work » Needs review
FileSize
features-bundle-2615634-7.patch893 bytes

  • nedjo committed 8503f46 on 8.x-3.x 
    Issue #2615634 by nedjo: clean up remaining bundle call using empty...
nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo at Chocolate Lily commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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