Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#44098 closed defect (bug) (fixed)

Widget classes when custom widget class is namespaced

Reported by: rogerlos's profile rogerlos Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.4.2
Component: Widgets Keywords: has-patch needs-testing needs-dev-note
Focuses: Cc:

Description

Widgets output from a namespaced class have a class added to the before_widget argument in a way which is potentially confusing and messy to work with.

The before_widget value within the arguments array sent to the widget() method within WP_Widget looks something like this when using a namespaced custom widget class:

<aside class="widget \myplugin\My_Widget" id="mywidgetname_widget-1">'

Given my understanding of WP's "normal" escaping of attributes, I would have expected myplugin-My_Widget, mywidgetname myplugin-My_Widget or maybe mypluginMy_Widget.

I believe that technically the two escape characters make the WP output the equivalent to the latter, but it's a bit messy and potentially confusing to folks looking to style the output, or find the aside using JS.

Attachments (3)

bug.png (46.1 KB) - added by Mte90 4 years ago.
Whan happens when the widget is a class initialized by a namespace without id_base defined
44098.diff (3.0 KB) - added by SergeyBiryukov 3 years ago.
44098.2.diff (3.6 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (25)

#1 @welcher
5 years ago

  • Keywords reporter-feedback added

@rogerlos thanks for the ticket! Can you share the code you are using to register the widget? My local tests are not getting the same results.

#2 @Mte90
4 years ago

  • Keywords needs-patch added; reporter-feedback removed
  • Version set to 5.4.2

I can confirm the issue.
This happens when you don't define the id_base that is automatically generated.

<?php
// https://developer.wordpress.org/reference/classes/wp_widget/
// line 163
 $this->id_base         = empty( $id_base ) ? preg_replace( '/(wp_)?widget_/', '', strtolower( get_class( $this ) ) ) : strtolower( $id_base );

So it use the class to generate it but if it is a namespace generated namespace\sub\sub\class doesn't do anything creating this issues.

@Mte90
4 years ago

Whan happens when the widget is a class initialized by a namespace without id_base defined

This ticket was mentioned in PR #435 on WordPress/wordpress-develop by Mte90.


4 years ago
#3

  • Keywords has-patch added; needs-patch removed

As https://core.trac.wordpress.org/ticket/44098 this happens when the widget is initialized without defining the id_base that is auto generated. The problem is that doesn't handle namespaces so this can create warning by php because the preg_match include a string with a backslash \.

Trac ticket:

#4 @Mte90
4 years ago

  • Keywords dev-feedback needs-testing needs-unit-tests added

I did a first patch version, I added a test but require some review.

Last edited 4 years ago by Mte90 (previous) (diff)

Mte90 commented on PR #435:


4 years ago
#5

This should fix the linting but for the test failing I don't know how I can mock a fake namespace and class.

#6 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.8

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

hmfs commented on PR #435:


3 years ago
#9

Hello @Mte90 ,

I am a new (hopeful) contributor. I saw this Trac ticket posted in the WordPress core slack room.

I was able to get the test to work by adding a namespaced widget. It seems cleaner to do this in a separate file than to try to add it to the original widget test file. I also had to adjust the logic in WP_Widget because end will fail the test when its argument is not a variable.

The code is available here.

Please let me know if you have questions.

Mte90 commented on PR #435:


3 years ago
#10

Hi @hmfs I merged your patch in this pull request.
Let's see if it is good now :-)

Mte90 commented on PR #435:


3 years ago
#11

@hmfs now the unit tests fails Fatal error: Class 'Namespace\Sub\Sub\Class' not found in /var/www/src/wp-includes/class-wp-widget-factory.php on line 61

hmfs commented on PR #435:


3 years ago
#12

Hi @Mte90 ,

Thanks for the notice.

The test in tests/phpunit/tests/widgets.php should be removed because it was migrated into the first test in tests/phpunit/tests/widgets-namespaced.php due to the complexity of adding multiple namespaces per file.

It also looks like the coding standards are complaining about the structure of widgets-namespaced.php.

Both issues are fixed in this new PR to this branch: https://github.com/Mte90/wordpress-develop/pull/4

Mte90 commented on PR #435:


3 years ago
#13

Perfect :-D

#14 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#15 follow-up: @SergeyBiryukov
3 years ago

Thanks for the PR!

Just noting that this also has backward compatibility concerns, as the id_base value is not only used for the widget's id and class attributes, but also for the option name which stores the widget settings (unless the widget specifies a custom option_name value).

If we only take the last part of the widget class name as id_base, it could conflict with a widget of the same name from another namespace, or a non-namespaced widget of the same name.

So it seems better to take the whole widget class name (including the namespace) and convert backslashes to hyphens, that way we should get a unique value.

That might still cause some previously saved namespaced widgets to disappear or lose their settings due to the option name change, but let's try this during the beta phase and see if we have any reports of that.

If we do get any reports, we might want to consider only doing this for the classname value and not for id_base.

#16 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 50953:

Widgets: Make sure WP_Widget constructor creates a correct id_base value for a namespaced widget class.

The id_base value is used for the widget's id and class attributes and also for the option name which stores the widget settings (unless the widget specifies a custom option_name value).

With this change, any backslashes in the id_base for a namespaced widget class are converted to hyphens, making it easier to style the output or target the widget with JavaScript.

This also avoids a preg_match(): Compilation failed PHP warning from next_widget_id_number() on the Widgets screen, previously caused by unescaped backslashes.

Props Mte90, hermpheus, rogerlos, welcher, SergeyBiryukov.
Fixes #44098.

#17 @SergeyBiryukov
3 years ago

  • Keywords needs-dev-note added; dev-feedback needs-unit-tests removed

#18 in reply to: ↑ 15 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to SergeyBiryukov:

That might still cause some previously saved namespaced widgets to disappear or lose their settings due to the option name change, but let's try this during the beta phase and see if we have any reports of that.

If we do get any reports, we might want to consider only doing this for the classname value and not for id_base.

On second thought, if the only concerns here are the class name and the PHP warning, it seems like a safer approach to just address that, instead of changing id_base. Since the handbook has an example with a namespace, it might be too common a pattern in the wild to cause any unnecessary breakage.

Let's also switch to underscores to avoid a mix of underscores and hyphens. See 44098.diff.

@SergeyBiryukov
3 years ago

#19 @Mte90
3 years ago

Seems a better solution that patch.

#20 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50961:

Widgets: Make sure WP_Widget constructor creates a correct classname value for a namespaced widget class.

This reverts the changes to id_base from [50953] due to backward compatibility concerns, and instead focuses on the id and class attributes specifically.

With this change, any backslashes in the id or class attributes for a namespaced widget class are converted to underscores, making it easier to style the output or target the widget with JavaScript.

Follow-up to [50953].

Fixes #44098.

Mte90 commented on PR #435:


3 years ago
#21

Ticket resolved

#22 @hellofromTonya
3 years ago

#16773 was marked as a duplicate.

Note: See TracTickets for help on using tickets.