Support from Acquia helps fund testing for Drupal Acquia logo

Comments

facine created an issue. See original summary.

facine’s picture

Status: Active » Needs review

Status: Needs review » Needs work

joelpittet’s picture

+++ b/inline_conditions.module
@@ -812,9 +812,14 @@ function inline_conditions_get_info_by_type($entity_type, $parent_entity_type) {
+  // If the module doesn't exists retun empty array.
+  if (module_exists($module)) {
+    return array();
+  }

This part will not work because because the only call to this function expects it to be NULL. It's actually not filtering but grouping by module.

Ignore the test results (there are no tests to test against... yet) but this does need work.

joelpittet’s picture

Sorry for the delayed response, I wasn't getting emails for this project, now I am:)

facine’s picture

Hello again, sorry for the bad patch! (the rush...) I attach a new patch.

As the patch is too small I don't attach any interdiff.

Status: Needs review » Needs work
jkuma’s picture

Status: Needs work » Needs review

i'll review your work this week end facine, thanks for your patch !

jkuma’s picture

@Thank you facine for this patch. I've changed it a little bit, made it smaller. I removed the "module exists" condition because all modules references by inline conditions are necessarily enabled.

I've tested it and it's working like a charm! Moving to RTBC.

jkuma’s picture

Update the patch to handle the case when module filter term doesn't match any modules of filtered conditions, so we return an empty array().

joelpittet’s picture

Nit: "its set" should be "it's set".

Also it could be more concise like this:
$filtered_conditions = !empty($filtered_conditions[$module]) ? $filtered_conditions[$module] : array();

jkuma’s picture

joelpittet’s picture

Thank you @jkuma, I guess the return value should still be

Return an array of conditions keyed by module name.

should the condition be
array($module => $filtered_conditions[$module])?

I'm not sure... thoughts?

jkuma’s picture

Status: Needs review » Needs work

Hello joel, thanks for the review.

From my perspective, there are no reasons to key by entity type if the module filter is applied. It only made sense if we could specify more than one module as function parameter.

On another hand, it's also important to always have the same return structure. As I write these lines, I think ambiguity should be avoid here and I'll go on your side. I'm updating the patch to keep the same return structure.

joelpittet’s picture

Thanks, it's nice to know functions return the same return structure so you don't have to change the code dealing with the output when providing different input.

jkuma’s picture

jkuma’s picture

Status: Needs review » Reviewed & tested by the community

I've tested the patch and it doesn't break inline conditions and its implementation in commerce_discount. I'm committing it into dev branch.

Thank you joel for the reviews.

jkuma’s picture

Status: Reviewed & tested by the community » Fixed

Committed to dev branch. Thanks all!

  • jkuma committed c73a940 on 7.x-1.x
    Issue #2639662 by jkuma, facine, joelpittet:...
joelpittet’s picture

Thanks @jkuma

Status: Fixed » Closed (fixed)

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