Clean-up code base to conform core standards

#2824322: Add ability set labels for languages in dropdown within ticket are some DI related fixes. extract them and get onto dev line.

CommentFileSizeAuthor
#15 2889674-15.patch2.51 KBandypost
#15 interdiff.txt372 bytesandypost
#14 2889674-14.patch2.15 KBandypost
#10 2889674-10.patch9.43 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SKAUGHT created an issue. See original summary.

SKAUGHT credited andypost.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented

now, currently on dev line.

  • SKAUGHT committed 76229f7 on 8.x-1.x 
    Issue #2889674 by SKAUGHT, andypost: DI related cleanup

  • SKAUGHT committed 912825b on 8.x-1.x 
    #2889674 related.  ConfigFactory

  • SKAUGHT committed 59532c9 on 8.x-1.x 
    #2889674 related.  ConfigFactory
SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Version: 8.x-1.1 » 8.x-1.x-dev

SKAUGHT credited Antonnavi.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Title: DI related cleanup » General code clean-up
Issue summary: View changes
Status: Active » Needs review
FileSize
2889674-10.patch9.43 KB

Here's a general set of fixes for the module

While checking this I fixed
- added config schema for settings
- fixed DI for block
- used proper "inline_template" for added help text

Needs follow-up issues
- add cache dependency on module's config to block ad get rid of render cache invalidation
- simplify settings form (I guess global settings should be moved to block level)

  • SKAUGHT committed f294d2c on 8.x-1.x authored by andypost 
    Issue #2889674 by andypost, SKAUGHT, Antonnavi: General code clean-up
SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented

#10 commited. thanks very much!

re: follow ups.
cache: myself, i'm still not familiar enough with the cache system to handle it better.
settings form: i myself was thinking about whether or not moving it to block level would be appropriate.. as it is a 'global' control vs each instance. This should be it's own issue, rather than on this cleanup ticket.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented

have opened #2889957: Dropdown language block caching issues

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2889674-14.patch2.15 KB

thanks for quick reply!

One more thing - wrapper actually is boolean, so better to convert this setting to checkbox

Filed follow-up for settings form #2890099: Refactor settings form

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt372 bytes
2889674-15.patch2.51 KB

Fix caching like core does

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Looks after this it makes sense to create new release

schema change to boolean may need update hook for existing sites but testing shows that config is smart enough

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Title: General code clean-up » DI related cleanup
Status: Needs review » Fixed

am returning title and closing. this aspect of 'code cleanup' has been addressed.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Status: Fixed » Closed (fixed)
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Issues in "Fixed" state are automatically closed in 2 weeks but visible in tracker (useful to see latest fixed)