Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey’s picture

Status: Active » Needs review
FileSize
11.07 KB

Removing drupal_add_region_content() and drupal_get_region_content() is a possibility, although I am not sure modules need to be able to add content to maintenance pages. This is currently only possible through drupal_add_region_content(), but I believe there are currently no possibilities for modules to do so.

Ideal IMHO would be if maintenance pages are build/rendered/alterable through drupal_render().

Status: Needs review » Needs work

The last submitted patch, drupal_add_region_content-remove.patch, failed testing.

andypost’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

I think this function inherited from D6 drupal_set_content() but limited by maintenance page for some reason.
For D7 we are using hook_page_alter() for purpose of adding some data to a region.

We should change doc block to point this or change behaviour to allow it work as D6 way.

andypost’s picture

tagged

sonugoyalmca2006@yahoo.com’s picture

Version: 8.x-dev » 7.x-dev

i am using drupal D7, want to assign content in region using drupal_add_region_content, but content is not shoing in particular region. can you help?

q0rban’s picture

Version: 8.x-dev » 7.x-dev

hook_page_alter() is not a replacement for drupal_set_content(). drupal_set_content() could be used inside of other functions, after certain data has been retrieved for instance. A way to achieve this in D7 with hook_page_build() would be to store that data in a static variable in the same way that drupal_add_region_content() is doing, and then retrieve it in hook_page_build(). Not an ideal workflow.

q0rban’s picture

Version: 7.x-dev » 8.x-dev
andypost’s picture

+++ update.php	20 May 2010 13:16:26 -0000
@@ -31,11 +31,7 @@
-  update_task_list('select');

Any reason to remove this?

1 days to next Drupal core point release.

q0rban’s picture

Version: 7.x-dev » 8.x-dev

andypost said:

For D7 we are using hook_page_alter() for purpose of adding some data to a region.

Actually, now that I'm looking at it, if you want to add content to a region, you should be using hook_page_build(), not hook_page_alter(). It's like the difference between hook_menu() and hook_menu_alter(). If you want to add new menu items, you use hook_menu(). If you want to tinker with existing ones, you use hook_menu_alter().

digitalsweetspot’s picture

Version: 8.x-dev » 7.22
Status: Needs work » Needs review

I found a workaround to this issue for D7, and posted it here: http://api.drupal.org/comment/48353#comment-48353. Hopefully this helps out. I welcome any feedback.

David_Rothstein’s picture

Version: 7.22 » 8.x-dev
Status: Needs review » Needs work

Thanks for posting that, but this core issue needs to stay on Drupal 8 (backport to Drupal 7 is possible later, depending on what the fix is).

jwilson3’s picture

Removing a feature from Drupal because its broken feels like the wrong thing to do.

* The amount of code to make this functionality actually work again is trivial (see the following patch below).
* The recommended workaround in #8 does *exactly* what this code already does in core, and makes modules/custom code more confusing (having to set custom variables and alter regions).

Here is a trivial patch that adds this functionality back to D7. The only thing left to do here would be
a) review the methodology suggested in this comment and patch
b) update this patch for D8
c) write a few tests that prove that you can add content to regions.

PS. I need this code because I need to pull content out of a panel region and place it into a theme region (a concept originally coined by Dave Reid for D6). It entirely depends on this function call, because inside a ctools panel layout definition, you don't have access to hook_page_alter() or hook_page_build(), and this function is really a very clean solution.

jwilson3’s picture

Status: Needs work » Needs review
FileSize
497 bytes

aaaaand.... the patch. :/

Status: Needs review » Needs work
Issue tags: -Needs documentation, -API clean-up, -Needs backport to D7

The last submitted patch, 713462-drupal-add-region-content-D7.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 713462-drupal-add-region-content-D7.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs documentation, +API clean-up, +Needs backport to D7

The last submitted patch, 713462-drupal-add-region-content-D7.patch, failed testing.

jwilson3’s picture

Status: Needs work » Needs review
FileSize
717 bytes

Rerolled for D8, so bot stops complaining.

andypost’s picture

Issue tags: +Twig

How another piece of mark-up affects twig?

jwilson3’s picture

Issue tags: +Needs tests

Per #12.c

jwilson3’s picture

Issue tags: -Needs documentation +Novice

I don't think this needs additional documentation, if the idea is to fix this functionality as it was working in D6. Also, tagging as Novice for test writing.

jwilson3’s picture

I;ve updated the documentation for Assigning content to regions to reflect the fact that you need a patch from this issue to make it work in D7 and D8. After this gets in, we should remove that notice.

schrammos’s picture

Assigned: Unassigned » schrammos
Issue summary: View changes
Issue tags: +Amsterdam2014

we are testing the patch

Status: Needs review » Needs work

The last submitted patch, 19: 713462-drupal-add-region-content-18.patch, failed testing.

jwilson3’s picture

Assigned: schrammos » Unassigned
Issue tags: +Needs a reroll

Did this get anywhere in Amsterdam? Looks like it might need a reroll.

jwilson3’s picture

Issue tags: -Needs a reroll +Needs reroll
dawehner’s picture

Version: 8.0.x-dev » 7.x-dev

This api does not exist anymore in d8.

mgifford’s picture

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
497 bytes

Re-rolled patch #19 from recent 7.x branch.

Paul B’s picture

Status: Needs review » Needs work

The last submitted patch, 33: 713462-drupal-add-region-content-test-33.patch, failed testing.

Paul B’s picture

Status: Needs work » Needs review
FileSize
2.27 KB

The patch from #32 with test

mgifford’s picture

What's the best way to test this? The issue summary hasn't been updated yet making it hard to RTBC.

The code looks good from a quick review. It applies of course.

Paul B’s picture

Issue summary: View changes
jwilson3’s picture

@#36:

Easiest way to test this would be to write code that adds some text to a region.

Eg, add drupal_add_region_content('header', 'Welcome!'); inside the seven_preprocess_page(&$vars) function.

^ this is totally untested, so I'm not even 100% sure this will work as expected, but "in theory" it should work.

Paul B’s picture

Why in seven_preprocess_page()? Is there a reason why you shouldn't call the function in a page callback?

jwilson3’s picture

He asked for an example of how to test it. Sure, you can place the function call anywhere you want where it will run, however, #38 was the path of least coding to actually test the feature.

dcam’s picture

Title: drupal_add_region_content() not usable » Content added via drupal_add_region_content() is not added to pages
Status: Needs review » Reviewed & tested by the community

This looks ok to me and it does seem to be a regression from 6.x behavior. I haven't been able to determine whether that was intentional or not. Since there are reports of this being a regression I checked to see what was adding this content to the page in 6.x. theme_blocks() was doing it, along with adding the blocks to the page. It looks like this dual-responsibility was eliminated, but the additional content code was just deleted rather than moved elsewhere. I suspect this might have gotten lost in the shuffle of functions where theme_blocks() got changed to block_get_blocks_by_region() and some functionality got put into template_preprocess_page().

After finding all that out, I think template_preprocess_page() probably is the proper place to put the code, rather than adding it back in via one of Block's functions. It's probably not the best way to add random content to a region, but if we're keeping it in the 7.x API then it's true that this function doesn't do what it's supposed to do.

I tested the patch. Before applying it, I couldn't add content to a region with drupal_add_region_content(). After applying the patch the content appeared at the bottom of the region (which is where it would have appeared in 6.x). By the way, this wasn't testable by adding drupal_add_region_content() to seven_preprocess_page() as was suggested in #38. Maybe template_preprocess_page() had already run by that point. Instead I tested it by adding content to a taxonomy admin page callback.

There is a code formatting problem with the new test function, testDrupalAddRegionContent(), in theme.test. The entire function needs to be indented one level and there is an extra line after the function that should be deleted. If anyone is interested then they can roll a new patch with those changes. I'm going to go ahead and mark this RTBC, however, so that @David_Rothstein can go ahead and take a look at it. It's possible he can just go ahead and make those changes on commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 713462-drupal-add-region-content-35.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community
jwilson3’s picture

Thanks for RTBC!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.36 release notes

Hm, this is kind of a mess, but yeah, the function still exists so making it work seems like the right thing to do :)

Committed to 7.x - thanks! I fixed the formatting issues noted by @dcam in #41 on commit.

  • David_Rothstein committed a610b97 on 7.x
    Issue #713462 by jwilson3, Paul B, casey, sivaji@knackforge.com, dcam:...

Status: Fixed » Closed (fixed)

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