Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#19342 closed defect (bug) (fixed)

Call to undefined function convert_to_screen when using plugins with custom meta boxes

Reported by: jackdewey's profile jackdewey Owned by: rboren's profile rboren
Milestone: 3.3 Priority: normal
Severity: major Version: 3.3
Component: Template Keywords: has-patch commit
Focuses: Cc:

Description

In WP 3.3 Beta 3, activating a plugin that registers custom meta boxes makes the web site go to a blank screen with the error:

Fatal error: Call to undefined function convert_to_screen() in C:\Home\Projects\htdocs\wp-admin\includes\template.php on line 854

Looking at this line, it is trying to call the function convert_to_screen to convert the name of the screen that meta box should be displayed on, since it is passed as one of the function arguments.

In this specific case, it happened upon trying to activate the Link Library plugin (http://wordpress.org/extend/plugins/link-library/)

Attachments (1)

19342.diff (1.6 KB) - added by nacin 13 years ago.

Download all attachments as: .zip

Change History (16)

#1 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.3
  • Owner set to nacin
  • Severity changed from major to blocker
  • Status changed from new to reviewing

#2 follow-up: @nacin
13 years ago

  • Severity changed from blocker to major

Link Library is doing a very bad thing: require_once(ABSPATH . '/wp-admin/includes/template.php');

add_meta_box() is only available in the admin for a reason — it should be hooked into an admin hook, rather than force-requiring files.

The only thing we could do here is move convert_to_screen() back to template.php, but stuff would still break since the screen isn't available.

We should do a grep and warn plugin authors doing it wrong.

#3 @nacin
13 years ago

56 instances of ABSPATH.*includes.*template.php in the plugins directory. Who knows how many do it for add_meta_box().

./plugins/autopaginate/class-Autopaginate-Plugin.php:203:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/bilingual-linker/bilingual-linker.php:19:require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/bp-groupblog/themes/p2/inc/template-tags.php:46:	require( ABSPATH . '/wp-admin/includes/template.php' );
./plugins/bsuite/bsuite.php:717:		require_once( ABSPATH .'/wp-admin/includes/template.php');
./plugins/buddypress-courseware/groups/templates/helpers/editor_helpers.php:36:    require_once ABSPATH . '/wp-admin/includes/template.php' ;
./plugins/buddypress-docs/includes/addon-taxonomy.php:547:	require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/bug-library/bug-library.php:42:require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/category-images-ii/class-Plugin.php:428:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/conference-schedule/plugin.php:429:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/cron-view/plugin.php:203:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/custom-field-template/custom-field-template.php:150:			require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/custom-post-template/plugin.php:185:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/dashbar/DashBar.php:223:			include_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/dashboard-widget-manager/dashboard-widget-manager.php:98:				require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/eletro-widgets/eletro-widgets.php:304:            require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/es-custom-fields-interface/es-custom-fields-interface.php:208:require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/feedwordpress/feedwordpress-walker-category-checklist.class.php:10:require_once(ABSPATH.'/wp-admin/includes/template.php');
./plugins/foliopress-wysiwyg/fckeditor/editor/dialog/internal-link.php:16:require_once(ABSPATH.'wp-admin/includes/template.php');
./plugins/google-website-optimizer-for-wordpress/gwo4wp.php:52:			require_once(ABSPATH . 'wp-admin/includes/template.php'); // Needed for add_meta_box()
./plugins/headline-split-tester/headline-split-test.php:52:            require_once(ABSPATH . 'wp-admin/includes/template.php'); // Needed for add_meta_box()
./plugins/headline-split-tester/trunk/headline-split-test.php:52:            require_once(ABSPATH . 'wp-admin/includes/template.php'); // Needed for add_meta_box()
./plugins/hm-portfolio/HM Core/cwp-framework/cwp.classes.php:687:		include_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/intensedebate/intensedebate.php:1429:		include_once ABSPATH . '/wp-admin/includes/template.php';
./plugins/leaguemanager/admin/admin.php:20:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/link-library/link-library.php:53:require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/media-categories/media-categories.php:260:			if (!function_exists('register_column_headers'))	require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/mediaburst-ecommerce-sms-notifications/class-plugin.php:465:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/mediaburst-email-to-sms/class-Plugin.php:428:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/modal-dialog/modal-dialog.php:28:require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/ninja-announcements/wp-editor-return.php:138:				include_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/ninja-galleries/wpnj-media-tags/mediatags_admin.php:355:		require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/old-custom-fields/old-custom-fields.php:126:	include_once( ABSPATH.'wp-admin/includes/template.php' );
./plugins/photoblog/codigos.php:7:require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/pods/ui/input_fields.php:104:        require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/pods/ui/input_fields.php:167:            require_once(realpath(ABSPATH . '/wp-admin/includes/template.php'));
./plugins/pods/ui/wp-editor/wp-editor.php:134:				include_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/projectmanager/admin/admin.php:20:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/qodys-fb-meta/plugin.php:22:	require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/qodys-fb-meta/plugin.php:231:					require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/qodys-owl-emporium/ajax/install_plugin.php:10:	require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/qodys-redirector/plugin.php:22:	require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/qodys-redirector/plugin.php:235:					require_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/realpress-real-estate-plugin/realpress.php:18:require_once(ABSPATH.'wp-admin/includes/template.php');
./plugins/theme-my-profile/theme-my-profile.php:85:                require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/tweet-images/category-walker.php:21:require_once( ABSPATH . '/wp-admin/includes/template.php' );
./plugins/tweet-images/plugin.php:379:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/twitter-tracker/plugin.php:191:		require_once( ABSPATH . 'wp-admin/includes/template.php' );
./plugins/twshot-for-wordpress/twshot-for-wordpress.php:387:   include_once ABSPATH.'wp-admin/includes/template.php';
./plugins/user-link-feed/user-link-feed.php:30:	require_once(ABSPATH . '/wp-admin/includes/template.php');
./plugins/worker/core.class.php:364:            @include_once ABSPATH . 'wp-admin/includes/template.php';
./plugins/worker/installer.class.php:23:		include_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/wp-comment-remix/ajax_comments.php:21:require_once(ABSPATH.'wp-admin/includes/template.php');
./plugins/wpg3/wpg3.php:392:      require_once(ABSPATH.'wp-admin/includes/template.php');
./plugins/wpzeus-worker/core.class.php:307:            include_once ABSPATH . 'wp-admin/includes/template.php';
./plugins/wpzeus-worker/installer.class.php:23:		include_once(ABSPATH . 'wp-admin/includes/template.php');
./plugins/zenphoto-gallery/zenphoto_gallery.php:31:  require_once(ABSPATH.'/wp-admin/includes/template.php');

@nacin
13 years ago

#4 @nacin
13 years ago

19342.diff should prevent most errors. I used is_admin() but one could easily use class_exists().

#5 @ryan
13 years ago

19342.diff to avoid the fatal is fine. But as nacin says template.php is not safe to load this way.

#6 @nacin
13 years ago

  • Keywords has-patch commit added
  • Owner changed from nacin to rboren

Okay, Ryan is working on better _doing_it_wrong() language for 19342.diff. And yeah, he knows about the missing bracket.

#7 @nacin
13 years ago

Made a note at #19345 for a wpdevel post.

#8 @ryan
13 years ago

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

In [19428]:

Move convert_to_screen() back to template.php to avoid fatal errors with plugins and themes that direct include template.php into the front end. Flag this bad behavior with _doing_it_wrong(). Props nacin. fixes #19342

#9 @nacin
13 years ago

In [19450]:

Use class_exists() rather than is_admin() as otherwise the unit tests won't work without a WP_ADMIN hack. see #19342.

#10 in reply to: ↑ 2 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

Replying to nacin:

Link Library is doing a very bad thing: require_once(ABSPATH . '/wp-admin/includes/template.php');

While add_meta_box() is not one of them, some of the functions in /wp-admin/includes/template.php can be useful outside the admin. Are you saying that require_once() is bad for this specific file, or for any file within /wp-admin?

I ask this more specifically because we currently have a plugin that uses require_once() to load /wp-admin/includes/image.php because we need to use wp_crop_image(), and also /wp-admin/includes/upgrade.php because we need to use dbDelta().

Is there another, preferred way to get access functions in files within /wp-admin/?

#11 follow-up: @dd32
13 years ago

While add_meta_box() is not one of them, some of the functions in /wp-admin/includes/template.php can be useful outside the admin. Are you saying that require_once() is bad for this specific file, or for any file within /wp-admin?

In general, for any admin file, outside of an admin context is bad practice, you shouldn't need to include files, except for certain files which are not normally loaded, such as wp-admin/includes/upgrade.php

I ask this more specifically because we currently have a plugin that uses require_once() to load /wp-admin/includes/image.php because we need to use wp_crop_image(), and also /wp-admin/includes/upgrade.php because we need to use dbDelta().

upgrade.php is a special one, since it's not generally loaded, and should only ever be included when needed, and only within an admin context.

image.php is a silly one, The image manipulation functions shouldn't really be in admin includes IMO, and as you know, if you need to do image alterations outside of the admin, you currently need to include it. Hopefully that can change whenever media is touched again.

That being said, You certainly shouldn't be performing image manipulation or database schema alterations in a front end request, it should be deferred until the admin is viewed, or to a cron entry or similar.. for the simple reason that multiple requests could cause a server overload very fast when you consider that many image functions in PHP require >100M of memory, and DB alterations can cause the same thing.

#12 in reply to: ↑ 11 @mikeschinkel
13 years ago

Replying to dd32:

In general, for any admin file, outside of an admin context is bad practice, you shouldn't need to include files, except for certain files which are not normally loaded, such as wp-admin/includes/upgrade.php

Thanks for the quick reply. So basically the answer is conceptually "everything in an admin file should only be needed in admin so that's why it's bad form to add outside of admin", correct? Of course, there's concept and reality, such as image.php, also correct?

So is require_once() okay within /wp-admin when not already loaded?

image.php is a silly one, The image manipulation functions shouldn't really be in admin includes IMO, and as you know, if you need to do image alterations outside of the admin, you currently need to include it. Hopefully that can change whenever media is touched again.

Does it make sense to make a ticket for moving the image functions outside?

That being said, You certainly shouldn't be performing image manipulation or database schema alterations in a front end request, it should be deferred until the admin is viewed, or to a cron entry or similar..

Only? I have image handling code that generates a sized image the first time one is called by URL, which can be a front-end URL, and it only ever generates the one file requested, and only the first time requested. So only files that are used are ever generated, unlike WordPress core which generates every size for every image upon upload. Do you assert that I'm doing it wrong with this approach? If I'm not mistake I've heard Otto say he wants to see generation of sized images on demand.

#13 follow-up: @dd32
13 years ago

Only? I have image handling code that generates a sized image the first time one is called by URL, which can be a front-end URL, and it only ever generates the one file requested, and only the first time requested.

This ticket isn't the place for such discussion, however, given the case where you're on a moderately trafficed site, how long does the image manip's take? How many other users visit the site and kick off similar manip requests for the same file while the first is generating? - It's that concurrency issue which you should be avoiding by pre-creating images. There are other tickets (which I don't have at hand) for dealing with all the above issues in both messages.

#14 in reply to: ↑ 13 @mikeschinkel
13 years ago

Replying to dd32:

Only? I have image handling code that generates a sized image the first time one is called by URL, which can be a front-end URL, and it only ever generates the one file requested, and only the first time requested.

This ticket isn't the place for such discussion,

Good point, so I won't continue the discussion other than to include the following, which I tried to post before I noticed you had replied:

Replying to dd32:

That being said, You certainly shouldn't be performing image manipulation or database schema alterations in a front end request, it should be deferred until the admin is viewed, or to a cron entry or similar..

FYI, turns out that @filosofo is loading `image.php` from the front end too (see line 119.)

#15 @nacin
13 years ago

If you need admin functions on the frontend, the only canonical way to do this is to include wp-admin/includes/admin.php. Otherwise you risk not including a dependency that either A) exists in a rarely used code block, or B) exists in a future version of WordPress.

Note: See TracTickets for help on using tickets.