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
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:

Comments

paulocs created an issue. See original summary.

paulocs’s picture

Assigned: paulocs » Unassigned
Status: Active » Needs review
StatusFileSize
new20.72 KB

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

colan’s picture

👍

marcusvsouza’s picture

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

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

Assigned: Unassigned » marcusvsouza

I will do the code review!

marcusvsouza’s picture

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

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

Assigned: Unassigned » paulocs

I'll provide a re-roll.

paulocs’s picture

Assigned: paulocs » Unassigned
Status: Needs work » Needs review
StatusFileSize
new20.58 KB

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

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

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

Assigned: Unassigned » paulocs

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

paulocs’s picture

Assigned: paulocs » Unassigned
Status: Active » Needs review
andregp’s picture

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

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.