see title. is trivial bug fix, so rtbc

CommentFileSizeAuthor
patch_123433 bytesmoshe weitzman

Comments

drumm’s picture

drumm
he/him
Location NY, US
commented
Category: bug » feature
Status: Reviewed & tested by the community » Needs review

I'd call this an API change. I won't commit it without further discussion about the impact on contrib modules being updated.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This would be easy to tell, by searching contrib for '_comment(' and seeing if the modules in question contained .info files. If not, they haven't been ported to HEAD yet. If so, see if they implement the form op.

My guess is the number of contrib modules affected would be next to nothing if this was done soon.

moshe weitzman’s picture

moshe weitzman commented
Category: feature » bug
Status: Needs review » Reviewed & tested by the community

we fixed loads of bugs like this after the feature freeze last time. further, we don't support multiple ways of doing things. i hope webchick's reply is sufficient. please postpone or commit this.

dries’s picture

dries commented

Let's figure out how many modules are using this ...

Why is this a 'bug'?

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Postponed

The last feature freeze isn't the best example a feature freeze we want to repeat.

Webchick just provided instructions for how to research this. Those would need to be acted upon.

This reminds me of the node module patch that removed a bunch of hooks and got reversed.

I think this is an okay patch, but I think it should wait for API development to open up again unless clearly demonstrated otherwise.

moshe weitzman’s picture

moshe weitzman commented

we avoid offerring multiple ways of doing same thing. this comment form is inconsistent with every other form in drupal (multiple ways to alter it). thus i call it a bug. others are welcome to call it a shoe or a tree or whatever they please.

are we trying to determine the number of modules that already upgraded to HEAD and kept this code in? that seems to me to be the only interesting number.

a module that uses the comment('form') hook just needs to move same code into its hook_form_alter() and remove a return. hardly onerous.

drumm’s picture

drumm
he/him
Location NY, US
commented

It is still an API change. No one has done any research into this (only how to do the research).

I'd be willing to commit well-written function documentation saying this is deprecated and alter should be used instead.

moshe weitzman’s picture

moshe weitzman commented
Version: x.y.z » 6.x-dev
Status: Postponed » Reviewed & tested by the community

still applies, with offset. and still a good idea.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
heine’s picture

heine commented
Status: Closed (fixed) » Active

This needs to be documented in the API docs and the update guide.

catch’s picture

catch
he/him
Primary language English
commented
Title: deprecate hook_comment('form') in favor of hook_form_alter() » document deprecation of hook_comment('form') in favor of hook_form_alter()
Component: comment.module » documentation
moshe weitzman’s picture

moshe weitzman commented
Assigned: moshe weitzman » Unassigned
Status: Active » Fixed

I just updated the API docs.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

dww’s picture

dww
we/he/they
commented
Title: document deprecation of hook_comment('form') in favor of hook_form_alter() » deprecate hook_comment('form') in favor of hook_form_alter()

*ahem*, finally added to the upgrade docs since I ran into this over at #361649: Issue comments broken in D6 HEAD:

http://drupal.org/node/114774/revisions/view/405000/421906

Also setting the title back for posterity...