There is no need for this function to be public. It will only be used by classes that extend this class

CommentFileSizeAuthor
#12 enableTheme-should-protected-2896366-12.patch804 bytesarunkumark
#3 enabletheme___should_be_protected_not_public_function_in_outsideinjavascripttestbase-2896366-3.patch804 bytescdmo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

cdmo’s picture

cdmo CreditAttribution: cdmo commented
Assigned: Unassigned » cdmo

I'll give this a shot!

cdmo’s picture

cdmo CreditAttribution: cdmo commented
Status: Active » Needs review
FileSize
enabletheme___should_be_protected_not_public_function_in_outsideinjavascripttestbase-2896366-3.patch804 bytes

That it?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@cdmo, yep thats it.

Besides letting someone else getting a commit credit the other reason I didn't do it myself is I can't mark my own patch RTBC.

So thanks!

cdmo’s picture

cdmo CreditAttribution: cdmo commented

Cool. Easy contribution process!

I'm off to add "Drupal Core Contributor" to my resume ;)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Nice catch! :) And thanks, @cdmo!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Assigned: cdmo » Unassigned

  • lauriii committed 72fadae on 8.4.x 
    Issue #2896366 by cdmo, tedbow: enableTheme() should be protected not...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Version: 8.4.x-dev » 8.3.x-dev

I definitely agree this should be protected instead of public. Making functions more private is always a small BC break. Because this only affects Settings Tray module which is still in Alpha, we are allowed to make this change.

Committed 67e1d78 and pushed to 8.4.x. Thanks!

Waiting for confirmation to commit this to 8.3.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: enabletheme___should_be_protected_not_public_function_in_outsideinjavascripttestbase-2896366-3.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs reroll
arunkumark’s picture

arunkumark
Location Coimbatore
CreditAttribution: arunkumark as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commented
Status: Needs work » Needs review
FileSize
enableTheme-should-protected-2896366-12.patch804 bytes

Hi,

I have re-rolled patch for the Drupal 8.x.3 version as per the comment #9

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Version: 8.3.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

8.3.x is now essentially criticals only, and this is in 8.4.x (and 8.5.x), so marking fixed. Thanks all!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks!

Status: Fixed » Closed (fixed)

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)