Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexanderpas’s picture

FileSize
13.27 KB

here it is...

alexanderpas’s picture

Status: Needs work » Needs review

improved docs some more, also changed block.admin.inc

alexanderpas’s picture

FileSize
15.13 KB

why do i seem to forget the patch???

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

We Currently have a testing bot failure. Hold on until it's fixed.

alexanderpas’s picture

hopefully i now get a working slave #4 :)

Dries’s picture

Status: Needs work » Needs review

Yes, please! :)

Anonymous’s picture

subscribe. #257032: Split block $ops into separate functions has stalled, and this would be a good first step.

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
38.46 KB

all tests should pass now.

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
40.39 KB

7659 passes, 0 fails, and 0 exceptions.

drewish’s picture

subscribing

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
44.48 KB

some small php error in the api documnetation... thanks botty!

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

someone can find that syntax error i made?

Dave Reid’s picture

Find it hard to believe this patch worked for you. :) When I applied it, all my blocks dissappeared. I went to admin/build/blocks and there were no blocks listed, so obviously something is not right. I tried to go to admin/build/testing but I got the following errors:
Parse error: syntax error, unexpected '}', expecting ')' in /home/davereid/Projects/drupal-head/modules/user/user.test on line 202
Parse error: syntax error, unexpected T_VARIABLE, expecting ';' or '{' in /home/davereid/Projects/drupal-head/modules/user/user.test on line 218

EDIT: Clearing my caches worked for making the blocks reappear in admin/build/blocks. I'm going to check if this works when running upgrade.php after patching.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
44.48 KB

Here's the patch without the syntax errors.

Dave Reid’s picture

Status: Needs review » Needs work

There are also some other patches (looks like the do-not-delete-user-1 patch) mixed in with this patch that do not belong. The 'syndicate', 'powered by drupal' and 'recent comments' blocks (node_block and system_block) have not been converted in this patch yet.

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
40.35 KB

thanks for finding the mixup i made... this is the patch that should be the one attatched...

Dave Reid’s picture

Status: Needs review » Needs work

Very nice work alexanderpas so far! Marking as code needs works as there is still a lot to complete:
- Convert book_block
- Convert comment_block
- Convert node_block
- Convert poll_block
- Convert profile_block
- Convert search_block
- Convert system_block
- Provide upgrade path? Even just something like:

/**
 * Refresh the block cache.
 */
function block_update_7000() {
  return array();
}

Adding that function to block.install helped the upgrade path and didn't make all my blocks disappear if I ran update.php after the patch from #21.

alexanderpas’s picture

thanks for that, will be updating those modules, and adding the update code.

also, will be adding some tests for those blocks ;)

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
70.19 KB

here we go, first patch with those modules converted... (let's see what botty makes of it.)

Anonymous’s picture

Status: Needs review » Needs work

nice work. some nitpicks follow:

get rid of 'the':

+ * Generates a list of the administrator-defined blocks.

this sentence doesn't make sense:

+ * Generates the configure for administrator-defined blocks.

do whatnow to davereid? ;-)

+          // Add to the list of blocks we return.drupal davereid

i think this docblock above book_block_list , book_block_save and book_block_configure can go:

  * Displays the book table of contents in a block when the current page is a
  * single-node view of a book node.

actually, there's a lot of repeated docblocs that need to go.

i don't think the new test in system.test adds much beyond what's in block.test. what would be a valuable addition is to implement the TODO in block.test:

    // Confirm that the block was moved to the proper region.
    // TODO: Implement full region checking.
    $this->assertText(t('The block settings have been updated.'), t('Block successfully moved to left region.'));
alexanderpas’s picture

Status: Needs work » Needs review
FileSize
84.04 KB

added some more tests, lessened the system.test, added regression tests (tests weither each block from a module is availble.) ;)

alexanderpas’s picture

slightly improved docblocks, full review please?

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

(11:23:10 PM) catch__: pwolanin: upload fail here: http://testing.drupal.org/pifr/file/1/no_op_in_hook_block_part2_1.patch
(11:24:51 PM) catch__: pwolanin: not good though.
(11:25:09 PM) alexanderpas: catch__: why on my patch??
(11:26:03 PM) catch__: alexanderpas: test bot trouble we think.
(11:29:54 PM) hunmonk: catch__: i don't understand the issue
(11:30:08 PM) hunmonk: catch__: the tests completed
(11:30:23 PM) catch__: hunmonk: yeah me neither, but the fails don't seem to be in HEAD.
(11:30:38 PM) catch__: hunmonk: or due to the patches.

Dries’s picture

Status: Needs work » Fixed

Committed to CVS HEAD. Thanks!

alexanderpas’s picture

Category: task » bug
Status: Fixed » Needs review
FileSize
13.12 KB

forgot to convert two modules, statistics and aggregrator, those are done now.

tests in those two modules: 331 passes, 0 fails, and 0 exceptions.

Anonymous’s picture

Status: Needs review » Needs work

looks good, one nitpick:

+  $result = db_query('SELECT fid, title FROM {aggregator_feed} WHERE block <> 0 ORDER BY fid');

i know it was there before, but can we change <> to != ?

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
13.12 KB

done!

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

looks good, all tests pass, RTBC.

drewish’s picture

Status: Reviewed & tested by the community » Needs review

justinrandell, i'd be hesitant to change the comparison operator with out testing this on postgresql first. i've gotten bug reports in some of my contrib modules in the past about it.

lets either drop that change out for now and go back to the patch in #31, or get someone to test this on pgsql.

Dave Reid’s picture

The '!=' operator will fail on PostgreSQL and is not acceptable by SQL-99. Please leave it as '<>'.

Dave Reid’s picture

Status: Needs review » Needs work
alexanderpas’s picture

Status: Needs work » Needs review
FileSize
13.12 KB

same patch as #31

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ahem, sorry for the noise.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS. Thanks for the follow-up.

drewish’s picture

Status: Fixed » Needs work

Uh someone needs to write some update docs: http://drupal.org/node/224333

drewish’s picture

turns out this was a duplicate of #257032: Split block $ops into separate functions but this one got committed...

Anonymous’s picture

nope, the refactoring issue is not a dupe - it aims for much bigger changes to the block system.

just needs to be brought back up to date.

alexanderpas’s picture

Status: Needs work » Fixed

added update documentation.

alexanderpas’s picture

Component: block.module » documentation
Status: Fixed » Active

last thing to update: block_example.module

RobLoach’s picture

I think we should really save the kittens here and split the #257032: Split block $ops into separate functions up into a number of separate issues. Removing $op was definitely the first step. Nicely done, everyone!

dropcube’s picture

alexanderpas’s picture

Assigned: alexanderpas » Unassigned
Status: Active » Needs review
FileSize
6.59 KB

block_example.module updated!

alexanderpas’s picture

Assigned: Unassigned » alexanderpas
Dries’s picture

There is no block_example.module in core :)

alexanderpas’s picture

drewish’s picture

alexanderpas, that's actually not part of core. it's in contrib so any developer with a cvs account can modify it: http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/exam...

alexanderpas’s picture

still, it needs to be updated... and i've got no CVS (don't want it yet!)

RobLoach’s picture

Add the documentation to block.api.php? Looks like there's already an exciting and amazing example in there :-P .

catch’s picture

Status: Needs review » Needs work

Yes let's add the extra detail to hook_block() in block.api.php, then we could probably do away with block_example.module entirely for 7.

alexanderpas’s picture

Title: remove $op from hook_block() » remove $op from hook_block(): Documentation
Assigned: alexanderpas » Unassigned
rfay’s picture

Assigned: Unassigned » rfay
rfay’s picture

Status: Needs work » Reviewed & tested by the community

As far as I can tell from a quick review, the goals that were agreed upon in #55 have been met. block.api.php is updated with the 5 current hooks and no reference anywhere to $op.

I recommend that we close this one. I don't believe there's any more work to do unless it's to remove block_example.module as had been proposed in #55.

rfay’s picture

Status: Reviewed & tested by the community » Fixed

Since it looks good to me, marking fixed.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

I just applied the patch from #47, with a few small modifications, to the contrib repository, to update the block example module for Drupal 7. http://drupal.org/cvs?commit=270062