Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Hi!, inline_conditions_get_info_by_module function doesn't filter by module name.
http://cgit.drupalcode.org/inline_conditions/tree/inline_conditions.modu...
attach a patch.
Regards!
Comments
Comment #2
facine CreditAttribution: facine as a volunteer commentedComment #5
joelpittetThis 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.
Comment #6
joelpittetSorry for the delayed response, I wasn't getting emails for this project, now I am:)
Comment #7
facine CreditAttribution: facine as a volunteer commentedHello 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.
Comment #9
jkuma CreditAttribution: jkuma as a volunteer commentedi'll review your work this week end facine, thanks for your patch !
Comment #10
jkuma CreditAttribution: jkuma as a volunteer commented@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.
Comment #11
jkuma CreditAttribution: jkuma as a volunteer commentedUpdate the patch to handle the case when module filter term doesn't match any modules of filtered conditions, so we return an empty array().
Comment #12
joelpittetNit: "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();
Comment #13
jkuma CreditAttribution: jkuma as a volunteer commented@joelpittet thanks for the review, updated patch accordingly.
Comment #14
joelpittetThank you @jkuma, I guess the return value should still be
should the condition be
array($module => $filtered_conditions[$module])
?I'm not sure... thoughts?
Comment #15
jkuma CreditAttribution: jkuma as a volunteer commentedHello 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.
Comment #16
joelpittetThanks, 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.
Comment #17
jkuma CreditAttribution: jkuma as a volunteer commentedI've updated the patch
Comment #18
jkuma CreditAttribution: jkuma as a volunteer commentedI'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.
Comment #19
jkuma CreditAttribution: jkuma as a volunteer commentedCommitted to dev branch. Thanks all!
Comment #21
joelpittetThanks @jkuma