The block API uses a set of magic variables (mainly 0, 1 and 2) that aren't defined anywhere. This makes the code less readable than it could be and makes the API itself less clean than it could be.

The block.install file referred to cache defines that no longer exist in the module, I've updated that. It also used hardcoded integers where it should have been using the DRUPAL_CACHE_* defines from common.inc

CommentFileSizeAuthor
#6 769540-3.patch10.07 KBcafuego
#4 769540-2.patch10.19 KBcafuego
#1 769540.patch9.68 KBcafuego

Comments

cafuego’s picture

cafuego commented
Category: feature » bug
Status: Active » Needs review
StatusFileSize
new 769540.patch9.68 KB

Attached patch updates block.install to use provided defines and adds some defines for a cleaner block API.

cafuego’s picture

cafuego commented
Issue tags: +API clean-up
casey’s picture

casey commented
Status: Needs review » Needs work

Looks good. You need to document constants separately however (have a look at node.module).

Also, I am not sure if this should be considered an API change (which then should go into D8).

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 769540-2.patch10.19 KB

Well, since the defined values are the same and the integers still work I'd like to think of it as a cleanup instead of an API change ;-)

3rd party modules will still work fine even if they don't use the defines - though this might encourage module developers to tidy up their code too.

Updated patch with per-define documentation (shamelessly copied from admin/block) attached.

Status: Needs review » Needs work

The last submitted patch, 769540-2.patch, failed testing.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 769540-3.patch10.07 KB

Fine, so I stuffed up a copy & paste. Fixed the patch, reattached, re-queued for testing.

casey’s picture

casey commented
Status: Needs review » Reviewed & tested by the community

Code is good. I don't see this as an API change either. Let's see what Dries/webchick thinks.

dries’s picture

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

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

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

Issue tags: +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.