Make WordPress Core

Opened 17 years ago

Closed 17 years ago

#4169 closed task (blessed) (fixed)

Bring widgets into WordPress core

Reported by: rob1n's profile rob1n Owned by: rob1n's profile rob1n
Milestone: 2.2 Priority: highest omg bbq
Severity: blocker Version:
Component: General Keywords:
Focuses: Cc:

Description

We need to do this before we can release 2.2. Basically just make Automattic Widgets part of the core.

Attachments (13)

widgets.diff (45.5 KB) - added by rob1n 17 years ago.
widgets.php (45.6 KB) - added by ryan 17 years ago.
wordpress.com version of widgets.php
wp-admin_widgets.php (470 bytes) - added by filosofo 17 years ago.
wp-admin_widgets.2.php (655 bytes) - added by filosofo 17 years ago.
wp-includes_widgets.php.diff20070428 (701 bytes) - added by filosofo 17 years ago.
auto-deactivate.diff (824 bytes) - added by rob1n 17 years ago.
Automattically disable Automattic Widgets if it's active
widgets.2.diff (2.9 KB) - added by andy 17 years ago.
undo last patch, no more using name as id
wp-includes_widgets.php.20070430.diff (3.7 KB) - added by filosofo 17 years ago.
move-stuff-around.diff (4.5 KB) - added by rob1n 17 years ago.
new-loading-widgets.diff (1.5 KB) - added by rob1n 17 years ago.
id-compat-fix.diff (5.2 KB) - added by rob1n 17 years ago.
new_registration.diff (11.2 KB) - added by ryan 17 years ago.
fracture.diff (12.0 KB) - added by ryan 17 years ago.
Back compat and rss fixes

Download all attachments as: .zip

Change History (85)

#1 follow-up: @rob1n
17 years ago

Things to consider:

  1. Which implementation we're going to integrate. I think the most viable choice here is the Automattic Widgets plugin (http://automattic.com/code/widgets/).
  1. Possible conflict with existing widget plugins. My proposed solution to work around this is to put wp_ before all those "internal" functions (i.e. not the template tags). Then, for the template tags, we do if (!function_exists()) checks so the internal implementation of widgets can:
    1. Be replaced by other plugins. Not everyone agrees on how something is implemented, and thus they would probably use a plugin.
    2. Work with existing plugins (so you won't get a fat fatal error when you upgrade).
  1. Where to put the widgets? Before, in the case of the Automattic plugin, the widgets were located in wp-content/plugins/widgets. I propose wp-content/widgets. It's the user's content, technically, and keeping it in wp-content keeps it independent of includes and admin.
  1. Where the admin menu will be. Either Manage > Widgets or Presentation > Widgets. Either way, but now that I think a bit more about it the second option makes more semantical sense.

That's about it, really. The default and classic themes are already widget-aware. Opening to debate.

#2 in reply to: ↑ 1 @filosofo
17 years ago

Replying to rob1n:

  1. Where to put the widgets? Before, in the case of the Automattic plugin, the widgets were located in wp-content/plugins/widgets. I propose wp-content/widgets. It's the user's content, technically, and keeping it in wp-content keeps it independent of includes and admin.

Why not let them be standalone plugins, if include them at all? That's what they are now (if you mean stuff like the del.icio.us widget).

#3 @rob1n
17 years ago

  • Owner changed from anonymous to rob1n

#4 @Otto42
17 years ago

Re: #2: Possibly we can check for and automatically deactivate the old widgets plugin as part of the upgrade.php? Might save some headaches.

Re: #3: Widgets are plugins, plugins can be widgets. There's no need to have a defined place to put widgets. The core code moves into the core, the widgets.php plugin file disappears, done and done. There's no need to move things around.

#5 @rob1n
17 years ago

That's the first part (60%, I'd say). What's left to do is the admin. That's it. Well, and fix the core part ;).

I'd appreciate it if people could look it over. In those 900 lines of code, I probably made a few mistakes. Hopefully not that many though.

#6 @matt
17 years ago

Don't forget to change the link in the meta widget to go to WordPress.org and not WordPress.com.

#7 @Otto42
17 years ago

In wp_widget_links();

This:

wp_list_bookmarks( array( 'title_before' => $before_title, 'title_after' => $after_title ) );

Should be this:

wp_list_bookmarks(array( 'title_before'=>$before_title, 'title_after'=>$after_title, 'category_before'=>$before_widget, 'category_after'=>$after_widget));

In order to make the links widget work properly.

#8 @technosailor
17 years ago

Big whopping +1 on this to be committed. Would be nice to be able to get fixed code in SVN for our blogs.

#9 @rob1n
17 years ago

Alright, new version that has Otto42 and matt's fixes.

#10 @rob1n
17 years ago

Note that there's a parse error. Some missing bracket or something. I'll track it down in the morning. (this is re widgets.php in wp-includes).

#11 @ryan
17 years ago

Extra close paren on line 16 of widgets.js.php. Warnings on lines 11 and 12 of wp-admin/widgets.php because $wp_registered_widgets is not initialized. wp-includes/widgets.php: $sidbar typo on line 59, $wp_registered_sidebar_defaults on line 133 does not match $wp_register_sidebar_defaults global, $items not initialized on line 771. Also, wp_widget_recent_entries() looks like it is missing a brace. I suggest just returning immediately if !have_posts() rather than putting the positive case inside an if block. Also, using the if:endif; style blocks for the function_exists() checks and not indenting the functions within the if:endif block is easier to read, IMO.

#12 @rob1n
17 years ago

Extra close paren on line 16 of widgets.js.php.

Warnings on lines 11 and 12 of wp-admin/widgets.php because $wp_registered_widgets is not initialized.

wp-includes/widgets.php: $sidbar typo on line 59

$wp_registered_sidebar_defaults on line 133 does not match $wp_register_sidebar_defaults global

$items not initialized on line 771.

Also, wp_widget_recent_entries() looks like it is missing a brace.

I suggest just returning immediately if !have_posts() rather than putting the positive case inside an if block.

Also, using the if:endif; style blocks for the function_exists() checks and not indenting the functions within the if:endif block is easier to read, IMO.

(Just getting it more like a checklist)

#13 @rob1n
17 years ago

  • Keywords has-patch needs-testing added

widgets-all.2.diff is what I've got so far. I've tested it lightly on my local testing server.

#14 @rob1n
17 years ago

Alright, we are good to go, as far as I can tell.

#15 @ryan
17 years ago

Looks good. No warnings in Eclipse.

@rob1n
17 years ago

#16 @rob1n
17 years ago

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

(In [5294]) Widgets support. fixes #4169 for 2.2

#17 @Sewar
17 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

We need widgets-rtl.css for RTL support.

#18 @Sewar
17 years ago

I will try to create patch and submit it later.

#19 @rob1n
17 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Let's split it up into different tickets.

#4185 is the RTL CSS ticket.

#20 @technosailor
17 years ago

Anyone with commit access care to commit Otto's change to the widget repository itself? Seems silly that the widget for Links hasn't been updated since WP 2.1 and bookmarks.

#21 @ryan
17 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Option names shouldn't get the wp_ prefix. We don't want to throw out sidebar provisioning made with the plugin.

#22 @ryan
17 years ago

Also, I see wp_widget_recent_entries() but not wp_widget_recent_entries_control() or wp_flush_widget_recent_entries().

#23 @ryan
17 years ago

Whoa, wp-includes/widgets.php has some ancient and screwy gettext bits in it. I see that the latest in the plugins svn is also ancient. Gotta go thump skeltoac.

@ryan
17 years ago

wordpress.com version of widgets.php

#24 @ryan
17 years ago

Attached the latest bits as used on wordpress.com. Notice that register_sidebar_widget() in this version doesn't try to gettext variables, which is a no-no.

#25 @ryan
17 years ago

(In [5301]) Update widgets to latest bits. see #4169

#26 @ryan
17 years ago

(In [5306]) Update widgets to latet bits. For 2.2. see #4169

#27 @filosofo
17 years ago

I've attached a patch to put the proper names of widgets (not their ids) back into the admin page.

#28 @rob1n
17 years ago

(In [5310]) Variable name fix. Props filosofo. see #4169

#29 @rob1n
17 years ago

(In [5311]) Variable name fix. Props filosofo. For 2.2. see #4169

#30 @Otto42
17 years ago

The latest patches once again broke the links widget... :(

Change:

wp_list_bookmarks(array('title_before'=>$before_title, 'title_after'=>$after_title, 'show_images'=>true, 'class'=>'linkcat widget')); 

to:

wp_list_bookmarks(array(
'title_before'=>$before_title, 'title_after'=>$after_title, 
'show_images'=>true, 'class'=>'linkcat widget',
'category_before' => $before_widget, 'category_after' => $after_widget 
)); 

And it's fixed.

#31 follow-up: @technosailor
17 years ago

The new widgets integration seems to be ignoring existing widgets. That's a big problem. :) It also does not play well when the widgets plugin is installed. We need to find a way to disable or ignore the widgets plugin or we're going to have lots of breakage.

r5313

#32 @rob1n
17 years ago

(In [5317]) Links widget fixes from Otto42. see #4169

#33 in reply to: ↑ 31 @rob1n
17 years ago

Replying to technosailor:

The new widgets integration seems to be ignoring existing widgets. That's a big problem. :) It also does not play well when the widgets plugin is installed. We need to find a way to disable or ignore the widgets plugin or we're going to have lots of breakage.

Do you mean existing widgets as in the ones that are in the wp-content/plugins folder, or the existing sidebar(s) setup? The DB option names are different, in order to coexist.

Also, any specifics on how the widgets plugin messes core widgets up?

#34 @technosailor
17 years ago

I mean the 3rd party widgets are active in the plugin panel, but do not show on the Sidebar Widgets drag and drop panel in wp-admin and don't show up on the blog itself.

As far as widgets in the core, I had to go in and delte the widgets plugin before the core widgets took over. Obviously, this is not the best scenario when we go gold.

#35 @filosofo
17 years ago

Widgets is ignoring sidebars that are named something other than the default, so that if you have a custom-named sidebar, none of its widgets appear on the appropriate pages (although it does appear in the admin section).

I've attached another patch ( wp-includes_widgets.php.diff20070428 ) that fixes this problem.

#36 @rob1n
17 years ago

(In [5343]) Move widgets' JS into the <head> of widgets.php. Should fix text/rss widget controls. see #4169

@rob1n
17 years ago

Automattically disable Automattic Widgets if it's active

#37 @rob1n
17 years ago

(In [5344]) Automattically deactivate Automattic Widgets, if activated. see #4169

#38 @rob1n
17 years ago

(In [5345]) Check basename(). see #4169

#39 @rob1n
17 years ago

(In [5348]) Bring back old behavior for sidebar ID's. Props filosofo. see #4169

@andy
17 years ago

undo last patch, no more using name as id

#40 @andy
17 years ago

On deeper review, I found [5348] to be more trouble. It's time to stop using Name for anything other than display.

If a sidebar is registered with no ID, it will be given a number and dynamic_sidebar() must be called with that number. If it has an ID, it must be called by that ID.

Numbered sidebars are ideal for most cases because they are more likely to convey when the theme is switched. If a theme uses IDs, other themes probably won't use them and the user will have different sidebars for different themes. If this is desired, use IDs.

#41 @andy
17 years ago

widgets.2.diff reverts [5348] and creates a new sidebars_widgets array version. The upgrade logic attempts to match orphaned sidebars to the ones registered by the theme.

#42 @filosofo
17 years ago

Can you explain why the status quo is trouble? It currently creates an ID from a sanitized version of the name, which doesn't strike me as being qualitatively different from "sidebar-" + an integer. It also seems easier to remember which is the "left sidebar" rather than which is "sidebar-2" or whatever.

#43 @andy
17 years ago

Yes. It's trouble because sometimes a name is translated or changed. If that happens, the id changes and this makes previously saved versions impossible to find.

This problem affects sidebars and widgets. That is, widgets registered with a translated name cannot be found after changing the translation or changing the blog's language. So widgets should be fixed as well. The fix I had for wordpress.com, and which was inadvertently copied into core, was to add a parameter similar to the sidebar id. I'm not sure how well that works right now, but it's the way to go. Names are for people to use and id's are for programs to use.

#44 @andy
17 years ago

If a theme has named sidebars, e.g. "Main sidebar" and "Bottom bar", the name parameter should still be used for these and it should be translated. The program needs a non-translated way to refer to the sidebar, so if you want to call dynamic_sidebar('main-sidebar') all you have to do is register your sidebar with that as that id.

Otherwise you pass an integer indicating the order in which the sidebar was registered. That works as well. Users don't see ID's, they see names.

#45 @ryan
17 years ago

(In [5358]) Widget registration fixes from Andy. For 2.3. see #4169

#46 @ryan
17 years ago

(In [5359]) Widget registration fixes from Andy. For 2.2. see #4169

#47 @filosofo
17 years ago

New problem: the maybe_disable_automattic_widgets function doesn't do any good, because what happens is that wp-includes/widgets.php gets included *before* the widgets plugin (hence all the checks in wp-includes/widgets.php for existing widgets functions are worthless), and it gets included *before* upgrade-functions.php has a chance to define maybe_disable_automattic_widgets. You end up with a fatal error, even when just requesting wp-admin/upgrade.php

So it makes sense to check for the widgets plugin in wp-includes/widgets.php then deactivate it.

My patch does that, and it removes the extraneous existing widget function checks.

#48 @rob1n
17 years ago

(In [5360]) Move widget stuff around. see #4169

#49 @rob1n
17 years ago

(In [5363]) New way of loading widgets that works with PHP 5. Thanks to filosofo for testing. see #4169

#50 @rob1n
17 years ago

Any other problems with this, so we can mark this as fixed?

#51 @ryan
17 years ago

Widgets written against the plugin version of widgets do this:

register_widget_control($name, $i <= $number ?
'widget_adsense_control' : /* unregister */ , 460, 350, $i);

Insead of:

register_widget_control($name, $i <= $number ?
'widget_adsense_control' : /* unregister */ , 460, 350, $id, $i);

$i is being interpreted as an ID and mucking things up.

Distinguishing the intent of that fifth arg, whether it be
an ID or an arbitrary arg being passed along, is difficult. Do we
need to have separate funcs for old and new?

Old:
function register_widget_control($name, $control_callback, $width =
300, $height = 200, ...)

New:
function wp_register_widget_control($id, $name, $control_callback,
$width = 300, $height = 200, ...)

The same for [wp_]register_sidebar_widget(). And update the
plugin with all of the changes we've done in core so that 2.0 and 2.1
users have it.

#52 @rob1n
17 years ago

  • Status changed from reopened to new

#53 @rob1n
17 years ago

  • Status changed from new to assigned

#54 @ryan
17 years ago

Let's do this. Remove the $id from register_widget_control() and register_sidebar_widget() to restore them back to the old way. Make them wrappers around the new functions wp_register_widget_control() and wp_register_sidebar_widget(). They will look like:

function wp_register_widget_control($id, $name, $control_callback, $width = 300, $height = 200, $args = array())

function wp_register_sidebar_widget($id, $name, $output_callback, $classname = , $args = array())

Change default widgets to use the new API.

#55 @ryan
17 years ago

Alternatively:

function wp_register_widget_control($id, $name, $control_callback, $args = array())

Collapse height and width into args.

@rob1n
17 years ago

#56 @ryan
17 years ago

That patch fixes back compat for me. RSS and recent comments widgets aren't saving their options, though. Also, we need to do an attribute_escape() audit.

@ryan
17 years ago

Back compat and rss fixes

#57 @ryan
17 years ago

RSS widget fixed. Registration API fractured for back compat support.

#58 @ryan
17 years ago

(In [5398]) Fix widget registration back compat by splitting API. Fix feed widget. For 2.2. see #4169

#59 @ryan
17 years ago

(In [5399]) Fix widget registration back compat by splitting API. Fix feed widget. For 2.3. see #4169

#60 @ryan
17 years ago

(In [5400]) Make sure Widgets menu comes after Themes. see #4169

#61 @ryan
17 years ago

(In [5401]) Make sure Widgets menu comes after Themes. see #4169

#62 @ryan
17 years ago

(In [5402]) attribute_escape for widgets. see #4169

#63 @ryan
17 years ago

(In [5403]) attribute_escape for widgets. see #4169

#64 @johnbillion
17 years ago

Under Presentation -> Widgets the sidebar on the left is titled "Sidebar 1" implying there is more than one sidebar available. Is it possible to add more sidebars?

#65 @JeremyVisser
17 years ago

It is up to the theme to tell WP how many sidebars are in the theme. I think it's in the register_sidebar() function or something like that.

If there's only one sidebar, it probably shouldn't be labelled 'Sidebar 1'.

#66 @Otto42
17 years ago

Themes can define specific names of the sidebars if they want to do so. If they don't, then you get the default "Sidebar X" stuff.

I say let the theme deal with it. A well made theme will have its own sidebar names anyway.

#67 @rob1n
17 years ago

Yep.

#68 @ryan
17 years ago

Someone wrote in with this report.

There is a bug in your widgets.php code. Around line 464 it says:



var left = $('shadow').offsetWidth - (el.offsetWidth + left);



I guess this should be:



var left = $('shadow').offsetWidth - (el.offsetWidth + right);

#69 @Otto42
17 years ago

That would be line 99 in wp-admin/widgets.php, and yes, he is correct. That line of code will cause a javascript error of some sort, and his fix is correct as well.

#70 @ryan
17 years ago

(In [5436]) Left, right, dragChange, hop. s/left/right/. Props Lawrence Pit. see #4169

#71 @ryan
17 years ago

(In [5437]) Left, right, dragChange, hop. s/left/right/. Props Lawrence Pit. see #4169

#72 @ryan
17 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.