Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specification of multiple Features bundles #627

Merged
merged 1 commit into from
Nov 8, 2016
Merged

Allow specification of multiple Features bundles #627

merged 1 commit into from
Nov 8, 2016

Conversation

timcosgrove
Copy link
Contributor

#626

We would like to be able to specify multiple Features bundles.

I would have liked to change the property name to cm.features.bundles but left it as-is for backwards compatibility sake. I've confirmed that the ForEachTask is perfectly happy with a single item.

@timcosgrove
Copy link
Contributor Author

#625 will most likely allow tests on this to pass. Will rebase once that is merged.

@grasmash grasmash added the Enhancement A feature or feature request label Nov 4, 2016
@grasmash
Copy link
Contributor

grasmash commented Nov 4, 2016

Restarting tests. Rebase should not be necessary.

<drush command="fra" assume="yes" alias="${drush.alias}">
<option name="bundle">${cm.features.bundle}</option>
</drush>
<foreach list="${cm.features.bundle}" param="bundle" target="setup:update:features-import" />
Copy link
Contributor

@grasmash grasmash Nov 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timcosgrove I believe that this changes requires cm.features.bundle to be an array (it can never be a string).

If that is the case, we should change the variable name to cm.features.bundles and change the example here to be an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grasmash I tested it with yml formatted as such:

cm:
  features:
    bundle: my_feature_bundle

This worked as expected, feeding into the foreach as an array with a single value.

I think 'bundles' is clearer myself but I wanted to not breaking existing project.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing confirmed the following forms all feed validly to the ForEachTask:

single:

cm:
  features:
    bundle: my_feature_bundle

comma-delimited:

cm:
  features:
    bundle: my_feature_bundle, my_other_feature_bundle, my_last_feature_bundle

per-line:

cm:
  features:
    bundle: 
      - my_feature_bundle
      - my_other_feature_bundle
      - my_last_feature_bundle

My personal preferences is for the last option, but it looks like Phing is happy to take either form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to be clear I think 'bundles' is clearer semantically but again I wanted to make sure this didn't break anyone's build.

@grasmash
Copy link
Contributor

grasmash commented Nov 8, 2016

K, I'm going to merge as is for the moment but I will likely change to bundles. I am concerned about backwards compatibility--we need a way to roll out delta updates that take care of changes like this.

@grasmash grasmash merged commit 4d42447 into acquia:8.x Nov 8, 2016
@timcosgrove
Copy link
Contributor Author

A simple way to handle the property name change would be to make 'bundles' the default, and have a simple check somewhere that sets bundles = bundle if bundles is empty and bundle is set.

dreamfony pushed a commit to dreamfony/blt that referenced this pull request Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants