Problem/Motivation

menu_test_theme_page_callback() is marked as @deprecated.

Proposed resolution

Remove the usages of menu_test_theme_page_callback() as well as the function itself.

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because @deprecated code is buggy.
Issue priority Normal because the code is functional.
Prioritized changes Prioritized because we are removing a @deprecated function.
Disruption Totally not disruptive because it's not widely called.
CommentFileSizeAuthor
#18 interdiff-16-18.txt496 bytessdstyles
#18 2563801-18.patch4.69 KBsdstyles
#2 remove-usage-menu-test-page-callback-2563801-1.patch1.44 KBrashid_786
#5 remove-usage-menu-test-page-callback-2563801-5.patch1.42 KBrashid_786
#7 interdiff-2563801-5-7.txt1.29 KBrashid_786
#7 2563801-7.patch1.44 KBrashid_786
#10 2563801-10.patch1.43 KBkeopx
#10 interdiff_2563801-7-10.txt1.22 KBkeopx
#15 2563801-15.patch3.27 KBjoshi.rohit100
#15 interdiff-15.txt2.97 KBjoshi.rohit100
#16 2563801-16.patch4.72 KBsdstyles
#16 interdiff-15-16.txt1.3 KBsdstyles

Comments

Mile23 created an issue. See original summary.

rashid_786’s picture

rashid_786 commented
StatusFileSize
new remove-usage-menu-test-page-callback-2563801-1.patch1.44 KB

filed an issue also to remove the callback itself #2564161.

rashid_786’s picture

rashid_786 commented
Status: Active » Needs review
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work
Related issues: +#2564161: remove @deprecated menu_test_theme_page_callback()

Thanks. :-)

A couple things:

  1. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,26 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   * @param bool $inherited
+   *   (optional) TRUE when the requested page is intended to inherit
+   *   the theme of its parent.
...
   public function themePage($inherited) {

    $inherited is documented as being optional, but isn't optional in the code. We should follow the code for this refactoring, so change the docs so $inherited isn't optional. 

  2. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,26 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   *   for the current page request. ¶

    Adds whitespace.

rashid_786’s picture

rashid_786 commented
Status: Needs work » Needs review
StatusFileSize
new remove-usage-menu-test-page-callback-2563801-5.patch1.42 KB

updated patch with suggested changes.

NikhilValand’s picture

NikhilValand commented 
  1. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,26 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   * @return string
...
-    return menu_test_theme_page_callback($inherited);
...
+    // Now we check what the theme negotiator service returns.

    In general, all lines of code should not be longer than 80 characters.
    https://www.drupal.org/coding-standards#linelength 

  2. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,26 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   *   A string describing the requested custom theme and actual theme being used

    In general, all lines of code should not be longer than 80 characters.
    https://www.drupal.org/coding-standards#linelength

rashid_786’s picture

rashid_786 commented
StatusFileSize
new interdiff-2563801-5-7.txt1.29 KB
new 2563801-7.patch1.44 KB

line length has been reduced less than 80 characters.

lachezar.valchev’s picture

lachezar.valchev commented

Hi all,

I will make a review on this today :)

BR.

lachezar.valchev’s picture

lachezar.valchev commented
Status: Needs review » Needs work

Hi,

The patch applies nice on the latest 8.0.x-dev.

However, when run it through the Coder, there are a couple of errors listed below.

  1. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,28 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   * Page callback: Tests the theme negotiation functionality.

    Doc comment short description must be on the first line. 

  2. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,28 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+   *   A string describing the requested custom theme and actual ¶

    Whitespace found at end of line. 

  3. +++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,28 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+    ->determineActiveTheme(\Drupal::routeMatch());

    Line indented incorrectly; expected 6 spaces, found 4.

keopx’s picture

keopx
Primary language Basque
Location Basque Country
commented
Status: Needs work » Needs review
StatusFileSize
new 2563801-10.patch1.43 KB
new interdiff_2563801-7-10.txt1.22 KB

Changes applied

oenie’s picture

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

Looks ok to me, RTBC ?

lachezar.valchev’s picture

lachezar.valchev commented

Hi,

Patch applies nicely and looks good to me as well.

Regards,
Lachezar

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -40,10 +40,27 @@ public function titleCallback(array $_title_arguments = array(), $_title = '') {
+    $theme_key = \Drupal::theme()->getActiveTheme()->getName();
...
+    $active_theme = \Drupal::service('theme.negotiator')

These are services - we should be injecting them into the controller properly rather than using \Drupal.

If MenuTestController extended ControllerBase this would be simply a matter of implementing a create method.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Also given this is a test method I think removal of menu_test_theme_page_callback() in this issue is fine too. It is test code.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Status: Needs work » Needs review
StatusFileSize
new 2563801-15.patch3.27 KB
new interdiff-15.txt2.97 KB
sdstyles’s picture

sdstyles commented
StatusFileSize
new interdiff-15-16.txt1.3 KB
new 2563801-16.patch4.72 KB

Also according to #14 removed menu_test_theme_page_callback()

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Status: Needs review » Needs work

Looks great, except for one thing:

+++ b/core/modules/system/tests/modules/menu_test/src/Controller/MenuTestController.php
@@ -7,12 +7,66 @@
+  }
+
+    /**
    * Some known placeholder content which can be used for testing.
    *

Improper docblock alignment.

Sooooooo close. :-)

sdstyles’s picture

sdstyles commented
StatusFileSize
new 2563801-18.patch4.69 KB
new interdiff-16-18.txt496 bytes

Fixed.

sdstyles’s picture

sdstyles commented
Status: Needs work » Needs review
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Diggit. :-)

RTBC unless it fails.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 17a9dac and pushed to 8.0.x. Thanks!

  • alexpott committed 17a9dac on 8.0.x 
    Issue #2563801 by rashid_786, sdstyles, keopx, joshi.rohit100, Mile23,...

Status: Fixed » Needs work

The last submitted patch, 18: 2563801-18.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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