Now that module block is optional (#367380: Make block module optional), there is no reason to have the block theme definition in system module, it should go in block module. Attached a patch.

CommentFileSizeAuthor
#9 369409-9-eojthebrave-theme_block_to_block_module.patch7 KBeojthebrave
Passed: 10931 passes, 0 fails, 0 exceptions View
#7 369409-6-eojthebrave-theme_block_to_block_module.patch5.53 KBeojthebrave
Failed: 10923 passes, 0 fails, 2 exceptions View
#5 theme_block_to_block.module-369409.patch7.84 KBdropcube
Passed: 10908 passes, 0 fails, 0 exceptions View
theme_block_to_block.module.patch8.26 KBdropcube
Failed: Invalid PHP syntax in includes/theme.inc . View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks reasonable to me. Tests pass, so RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

dropcube’s picture

Status: Needs work » Needs review

Still applies, don't know why Testbed is failing.

Status: Needs review » Needs work

The last submitted patch failed testing.

dropcube’s picture

FileSize
7.84 KB
Passed: 10908 passes, 0 fails, 0 exceptions View

Re-rolling the patch.

dropcube’s picture

Status: Needs work » Needs review
eojthebrave’s picture

FileSize
5.53 KB
Failed: 10923 passes, 0 fails, 2 exceptions View

Downloaded patch,

Applied but has CR endings. Should be unix line endings.

Other than that everything seems to work fine. Tried switching to the Stark theme which uses the default block.tpl.php and everything worked as expected.

I think this patch makes sense, no reason in having drupal load code that isn't being used if the block module is disabled. It's also a very simple patch, mostly just moving code from one file to another, doesn't add anything "new".

Attached patch fixes the line ending issues.

Status: Needs review » Needs work

The last submitted patch failed testing.

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
7 KB
Passed: 10931 passes, 0 fails, 0 exceptions View

Oops, looks like my patch failed to create the new file in modules/block/, I think this one should fix that. Changing to needs review so the test bot can have another go at it.

Assuming this passes testing I would say it is ready to be committed.

eojthebrave’s picture

Status: Needs review » Reviewed & tested by the community

I think this one is ready to go. Like I mentioned before the original patch is really simple and really only moves some code from one place to another. Doesn't add any new functionality and makes perfect sense now that block module is optional. My re-roll was only done to eliminate line ending errors that I got when trying to apply the patch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Seems logical to me! I also ran this by quicksketch and he pointed out that such a patch would allow betterblock.module to also define its own block.tpl.php so that themes wouldn't have to break. Seems like pretty straight-forward unfinished business from the making block module optional patch.

Unless I'm wrong, this doesn't seem to impact any themers directly, so can just be marked fixed without the need for update documentation, correct?

eojthebrave’s picture

Correct. This shouldn't have any implications for themers, the only change is that they would now need to look in modules/block/ to find the block.tpl.php default implementation instead of modules/system/. Which, as someone who will probably be looking for it in the future, that makes a whole lot more sense as a place to start looking.

Status: Fixed » Closed (fixed)

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