Problem/Motivation

As proposed in the parent issue of this issue, all functions in conditional_field.module (except hooks) are candidates to be moved to a service.

Proposed resolution

Create a service to cover the conditional_fields_element_after_build logic.

CommentFileSizeAuthor
#10 3217413-10.patch20.58 KBpaulocs
#2 3217413-2.patch20.72 KBpaulocs

Issue fork conditional_fields-3217413

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

paulocs created an issue. See original summary.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: paulocs » Unassigned
Status: Active » Needs review
StatusFileSize
new 3217413-2.patch20.72 KB

Notice that I didn't fix any code standard because it is out of the scope of this issue.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented

👍

marcusvsouza’s picture

marcusvsouza commented
Status: Needs review » Reviewed & tested by the community

The patch applies without problems and work as expected, does not affect or change module functioning.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Could someone do a code review? I just thumbs upped the idea, and don't have time right now.

marcusvsouza’s picture

marcusvsouza commented
Assigned: Unassigned » marcusvsouza

I will do the code review!

marcusvsouza’s picture

marcusvsouza commented
Assigned: marcusvsouza » Unassigned
Status: Needs review » Reviewed & tested by the community

The function logic of “function conditional_fields_element_after_build” was transferred to a service and having no change in its functionality, no code pattern work was done because it escapes the scope of the issue.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

I was hoping to commit this, but it no longer applies.

Can we get it ready to go again before committing anything else? It's a big and important change. Thanks!

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: Unassigned » paulocs

I'll provide a re-roll.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: paulocs » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3217413-10.patch20.58 KB

  • colan committed 94f2996 on 4.x authored by paulocs 
    Issue #3217413 by paulocs, colan, marcusvsouza: Moved all...
colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Needs review » Fixed

Thanks, committed! I re-queued all of the other 4.x RTBCs for testing to ensure they still work.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Fixed » Active

Actually, there are method names like conditionalFieldsIsPriorityField(), which should be renamed to drop the CF prefixes. For example, this one should be isPriorityField(). Let's keep this open until we can update these?

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: Unassigned » paulocs

Working on #13. I'll open a MR.

paulocs opened merge request !14

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: paulocs » Unassigned
Status: Active » Needs review
andregp’s picture

andregp commented
Status: Needs review » Reviewed & tested by the community

@paulocs' MR correctly updates all methods from ConditionalFieldsElementAlterHelper.php that starts with "conditionalFields...". Thanks!

I have a question though. There are some methods on Conditions.php that also have the CF prefixes (conditionalFieldsDependencyDefaultSettings, conditionalFieldsStates, conditionalFieldsEffects, conditionalFieldsConditions).
@colan, should they be updated too (on a separated issue)?

  • colan committed b7dd487 on 4.x authored by paulocs 
    Issue #3217413 by paulocs, colan, marcusvsouza: Move all...
colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Thanks all!

#17: Sure, that would be a good idea.

Status: Fixed » Closed (fixed)

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