Move the procedural helper functions to a manager. This is a previous step to add tests.

CommentFileSizeAuthor
#6 2610208-6.patch20.07 KBdevkinetic
#5 2610208-5.patch19.83 KBdevkinetic
#3 2610208-Move_procedural_functions_to_a_service-3.patch11.36 KBfjgarlin

Comments

e0ipso created an issue. See original summary.

ericduran’s picture

ericduran commented

+1 here.

Looking at the module now there's a lot of room for improvement. I'll see if I can find time to help here. I mostly came to this from a testing perspective.

fjgarlin’s picture

fjgarlin commented
Status: Active » Needs review
StatusFileSize
new 2610208-Move_procedural_functions_to_a_service-3.patch11.36 KB

Even though the issue is old, this is still applicable. I made an attempt to move all the helper functions into a service. Patch included. I tested the module and everything seemed to be working as expected.

devkinetic’s picture

devkinetic commented

I had just created #3319167: Refactor Toolbar code to use a helper class + add per environment permissions and then found this. The patch in that issue applies to 4.x.

devkinetic’s picture

devkinetic commented
Version: 8.x-3.x-dev » 4.x-dev
Priority: Normal » Critical
StatusFileSize
new 2610208-5.patch19.83 KB

Here is an updated patch, closing that other issue.

devkinetic’s picture

devkinetic commented
StatusFileSize
new 2610208-6.patch20.07 KB

The previous patch had a missing service.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed

This looks good! Thanks for the patch.

  • e0ipso committed 2ba6b50 on 4.x 
    Issue #2610208 by devkinetic, fjgarlin, e0ipso, ericduran: Move...

Status: Fixed » Closed (fixed)

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