Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ZenDoodles’s picture

Status: Active » Needs review
FileSize
1.67 KB
ZenDoodles’s picture

And one for 6.x-1.x

mikebell_’s picture

Tested the Drupal 6 version and works well. Couldn't apply the patch using drush make though (even though it said it was fine) so applied manual. Could be down the fact I'm already patching features as it is.

mbell@cheetah [10:03:54] [/var/www/html/mbell/html] [users *]
-> % drush fc
 Feature                        Conflict(s)   
 BR Controller (br_controller)  br_layout     
 BR Layout (br_layout)          br_controller 

When I resolved the conflict and ran drush fc again it still showed up as conflicted, not an issue with the patch but more an issue with features.

febbraro’s picture

Assigned: ZenDoodles » febbraro
scottrigby’s picture

How about changing this function to drush_features_conflicts_all(), and reserve drush_features_conflicts() for more specific conflict info.

It could behave like this:

$ drush fca

+-----------------+---------------------------------+
| Feature         | Conflict(s)                     |
+-----------------+---------------------------------+
| custom          | ethosce_courses, ethosce_groups |
| ethosce_courses | custom                          |
| ethosce_groups  | custom                          |
+-----------------+---------------------------------+

Then we could drill down with drush "Features conflict FEATURE_NAME":

$drush fc custom

+-----------------+-------------------------------------+
| Conflict        | Components(s)                       |
+-----------------+-------------------------------------+
| ethosce_courses | node => course                      |
| ethosce_courses | menu_links => primary-links:courses |
| ethosce_groups  | node => learning_group              |
| ethosce_groups  | imagecache => group_image           |
+-----------------+-------------------------------------+

Edit: simplified.

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

@scottrigby: I'm not sure that specific details conflict information is available. You are welcome to submit a patch to add this additional info. But for now I think the original patch is useful. And it doesn't preclude adding an optional argument to fc in the future (no need for 2 separate commands).

Patch in #1 applies correctly to 1.x D7 features and seems to work, so marking this as reviewed.

scottrigby’s picture

@mpotter I was thinking about consistency with existing function/drush commands like fr & fra, fu & fua…

mikebell_’s picture

I agree with @scottrigby. At least changing the function name will allow for the more specific "drush fc" later. It's also good to keep consistency.

hefox’s picture

Status: Reviewed & tested by the community » Needs work
+  $rows[] = array( dt('Feature'), dt('Conflict(s)'));

Rows is never initialized as array to begin with? Also, extra space array( dt(

+      $module_conflicts = array();
+      foreach (array_keys($conflicts[$name]) as $conflict) {
+        $module_conflicts[] = $conflict;
+      }
+      $rows[] = array(
+        "{$module->info['name']} ({$module->name})", // Name (machine_name)
+        implode(', ', $module_conflicts);
+      );
+    }

isn't that just =>

      $rows[] = array(
        "{$module->info['name']} ({$module->name})",
        implode(', ', array_keys($conflicts[$name]));
      );

Doing the foreach doesn't seem to be doing anything and foreaches are slow.

Also: agree with others about renaming the function

ericduran’s picture

I needed this on a project and I wrote the code before even looking in the issue queue :-/ and now that I was about to post the patch I search for it and found this issue :-/

So here's my patch, is completely different then the other patch and also doesn't take into account anyones comment because again I wrote it before looking in here.

:-/

I took a couple of different approached I list the feature name, the feature is conflicting with, the component and then the individual component name.

I attached a screenshot of what it display. Lol sorry for the blackout I was wayyy to lazy to set up a environment with conflicting features instead of just demoing what I already have.

ericduran’s picture

FileSize
18.2 KB

Actually here's an appropriate screenshot.

drush-fc.png

ericduran’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

Here's an updated patch with the recommendations mention above.

Note:
- I rename the function to drush feature-conflicts-all and shortcuts fca.
- I have it throw drush errors when there's conflicts. This helps if you run this command on a CI environment like Jenkins so the build can fail if someone commits conflicting features.
- I didn't make it take a parameters because this essentially shows everything you need to see in one screen.
- I know the foreach look horrible especially since I think is like 4 or 5 level deep but sadly this is because of the values return from features_get_conflicts. If someone knows away around that please let me know. Other wise I think this is good.

Changing the status as needs review as I think this last patch should be pretty good or at least close to done.

ericduran’s picture

Assigned: febbraro » Unassigned
mpotter’s picture

Status: Needs review » Reviewed & tested by the community

Applied to latest 7.x-1.x-dev version and seems to work fine. Marking this as Reviewed.

One enhancement to think about for the future is an option --enabled that only shows conflicts with enabled modules. When I clone a Feature and then disable the original, I know that it's going to cause conflicts, but I don't care about seeing them (and wouldn't want them to generate any errors)

mpotter’s picture

@ericduran: Also, please review this: http://drupal.org/patch/submit for patch file naming conventions.

ericduran’s picture

@mpotter, ha thanks for the heads up. I had no clue that patch naming format was an actual standard. I guess it does make sense for people using things like drush make. :)

Grayside’s picture

Status: Reviewed & tested by the community » Needs work

Why not just features-conflicts? features-conflicts-all is a fairly long string, and a more specific use case (if currently the only use case) than we necessarily need to codify into the command name.

+++ b/features.drush.incundefined
@@ -88,6 +88,11 @@ function features_drush_command() {
+  $items['features-conflicts-all'] = array(

@@ -115,6 +120,8 @@ function features_drush_help($section) {
+    case 'drush:features-conflict-all':

Mismatched pluralization.

scottrigby’s picture

Yeah, if it's showing component & component name already i can't see any reason why we wouldn't just stick with features-conflicts like zendoodles initially suggested. I just wanted to leave open the space for that kind of detail.

ericduran’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Ok, now we have both patches one with feature-conflicts and one with feature-conflicts-all :)

So this should be good to go.

hefox’s picture

Hah, this works and applies (mostly, the help got rejected) to d6.

Using it on a code base that has many conflicting features, having an option to specify what feature would be quite nice (and could go into a separate feature request after this is committed, but it'd prob be easy to quickly add it here). I'd imagine drush features-conflicts by itself list all features, and drush features-conflicts [feature] would show just that feature's conflicts (not sure if drush features-conflicts [feature_a] [feature_b] would show conflicts between feature_a and feature_b or all conflicts for both features.. huh).

hefox’s picture

Status: Needs review » Needs work

Drush fc got taken by drush features-components :-/

Grayside’s picture

fcon?

Kristen Pol’s picture

I just applied #19 against 7.x-2.0-rc2 and got errors:

[zd7dev:kristen:features]$ patch -p1 < features-1349608-19.patch 
patching file features.drush.inc
Hunk #1 succeeded at 120 (offset 32 lines).
Hunk #2 FAILED at 120.
Hunk #3 succeeded at 841 (offset 209 lines).
Hunk #4 succeeded at 892 (offset 209 lines).
1 out of 4 hunks FAILED -- saving rejects to file features.drush.inc.rej
mpotter’s picture

Patch needs to be re-rolled against latest -dev version to get reviewed and tested. When it's re-rolled, remember to switch the Version to 2.x-dev so the automated tester finds it.

undertext’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
FileSize
2.68 KB

Reroll