Remove Search box from theme system, default to block system instead

I agree. It seems silly to have it there.

The issue of having it as a block will be a hard one to overcome. Setting a message is possible, bust messages most often get ignored from my experience. Enabling it when activated is tricky in the fact we do not have a region specifically for it to go to. The best option is to have it active by default since I would expect a search functionality out of the box in a CMS.

I agree as well. I believe that the search box should be a block, in the blocks administration page, rather than just built into the theme.

This sounds right to me... If the Usability team approves it, I'll write the patch.

Adding "needs usability review" tag.

Good point,I would agree with that.Having the search box as a block is more appropriate then also including it in the themes.Saves us the hassle and gives us more total control over it under the block system.

- Yes, removing search as a theme setting makes a lot of sense. It's very confusing.
- Enabling search + search block in the default install profile sounds like a nice default for the 80% use case, too.

Yes, please.

Attached patch removes the search_box from the theme. It adds the search box to the garland theme on the default install profile; it does not add the search box to the minimal profile, because search.module isn't enabled in this profile.

Please review the message in search_install(). This message is intended for when the minimal install profile is used, and then the search.module is enabled.

I chose not to add a conditional check before the message, which could be used to only display the message if a search block doesn't already exist -- it's possible for someone to install and uninstall ... unless of course, the system deletes all blocks when a module is deleted. Do we do this automatically? Or should we do this manually here?

Awesome!

Just tested the patch and it looks good to me.

Only thing that could potentially be confusing is that the search block is actually called "Search form" on the blocks page. So, maybe this should be tweaked a little:

+ drupal_set_message(t('You can <a href="@url">add the search block</a> now.', array('@url' => url('admin/structure/block'))));

Maybe this would be better?
+ drupal_set_message(t('The search form block is now available on the <a href="@url">blocks page</a>.', array('@url' => url('admin/structure/block'))));

Looks good. Committed to CVS HEAD. Thanks.

Re @Jacine in #9: feel free to follow-up with better wording still. Just re-open the issue when you have a patch.

Actually going to mark this as 'code needs work' because this requires an update to the upgrade instructions. Contributed themes might need to be modified.

I added something here: http://drupal.org/update/theme/6/7

My comments were short, and hopefully will be wordsmithed by someone on the documentation team.

Is there anywhere else this should be added?

All necessary info is in there, made sense to me. Thanks.

Here's a patch for install message wording ;)

Nicely specific and to the point.

This was accidentally committed to HEAD, and now causes a fun message that pops up during the batch install process. User clicks that link, interrupts the batch process, and bingo, one messed up half-installed Drupal. Needs to be reverted. None of our other modules do a drupal_set_message() to let us know that we can add it's blocks, that's just kind of silly. This needs to be in an update function.

Salacious evidence: http://skitch.com/yoroy/nya82/picture-13

Patch to remove the message display during install process.

Now with 100% less trailing whitespace

for reference:

oops

.

How about we remove this message all together? We don't do something similar for other blocks. It just looks like an odd special case to me.

I have no opinion on if we should have the message or not. Original issue from webchick says the following, so that's what I did:

1 - Set a message alerting admin to enable the search block if search is enabled and the block is not.

But Less is better. I vote for no message.

Committed to CVS HEAD. Thanks.

good work..seems cool enough to me.. :-)

Shouldn't the toggle_search theme setting have been killed as well?

Good question. I think it should have been yes. Can't talk about code implications so needs review from someone who does.

Removed.

Committed to CVS HEAD. Thanks!

The toggle_search checkbox is actually still appearing in the UI and getting saved to the database.

I looked into this and it turns out that this is because Drupal has "theme features" which automatically get converted to toggles by prepending 'toggle_' in front of them (so it would have been missed if searching the codebase for 'toggle_search').

This patch removes the search theme feature and therefore actually gets rid of the toggle.

Nice catch. Committed to CVS HEAD. Thanks.