#44098 closed defect (bug) (fixed)
Widget classes when custom widget class is namespaced
Reported by: | rogerlos | Owned by: | 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)
Change History (25)
#2
@
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.
@
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
@
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.
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.
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
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.
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
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
#15
follow-up:
↓ 18
@
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
.
#18
in reply to:
↑ 15
@
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 forid_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.
@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.