Permalink
Browse files

fix(events): the pagesetup event timing is more like 1.x

In 1.x, the event was triggered just before the first view was rendered
and it was assumed that some global state, particularly context, had been
set by this time in page-specific code.

In 2.0 this assumption is wrong because the page-specific code that sets
state like context often runs *within* a resource view. Hence the event
is triggered too early, before the page-specific state has been set up.

Here we delay the `pagesetup` event if the first view is a resource view.

This allows plugins to move all page-specific logic like context setting
into a resource view with more confidence that that state will be available
in their `pagesetup` event handlers. This likely fixes a class of
undiscovered bugs like #9271 and #9322.

Refs #8374
  • Loading branch information...
mrclay committed Mar 3, 2016
1 parent 86a1a44 commit 38b122888df42599be0c1ab47333c356822fbb2f
Showing with 35 additions and 5 deletions.
  1. +35 −5 engine/classes/Elgg/ViewsService.php
@@ -65,6 +65,11 @@ class ViewsService {
* @var SystemCache|null This is set if the views are configured via cache
*/
private $cache;
+
+ /**
+ * @var bool
+ */
+ private $allow_delay_pagesetup = true;
/**
* Constructor
@@ -275,11 +280,7 @@ public function renderView($view, array $vars = array(), $bypass = false, $viewt
$view_orig = $view;
- // Trigger the pagesetup event
- if (!isset($GLOBALS['_ELGG']->pagesetupdone) && !empty($this->CONFIG->boot_complete)) {
- $GLOBALS['_ELGG']->pagesetupdone = true;
- _elgg_services()->events->trigger('pagesetup', 'system');
- }
+ $this->handlePageSetup($view);
// Set up any extensions to the requested view
if (isset($this->views->extensions[$view])) {
@@ -328,6 +329,35 @@ protected function fileExists($path) {
return $this->file_exists_cache[$path];
}
+ /**
+ * Trigger the system "pagesetup" event just before the 1st view rendering, or the 2nd if the 1st
+ * view starts with "resources/".
+ *
+ * We delay the pagesetup event if the first view is a resource view in order to allow plugins to
+ * move all page-specific logic like context setting into a resource view with more confidence
+ * that that state will be available in their pagesetup event handlers. See the commit message for
+ * more BG info.
+ *
+ * @param string $view View about to be rendered
+ * @return void
+ */
+ private function handlePageSetup($view) {
+ if (isset($GLOBALS['_ELGG']->pagesetupdone) || empty($this->CONFIG->boot_complete)) {
+ return;
+ }
+
+ // only first rendering gets an opportunity to delay
+ $allow_delay = $this->allow_delay_pagesetup;
+ $this->allow_delay_pagesetup = false;
+
+ if ($allow_delay && (0 === strpos($view, 'resources/'))) {
+ return;
+ }
+
+ $GLOBALS['_ELGG']->pagesetupdone = true;
+ _elgg_services()->events->trigger('pagesetup', 'system');
+ }
+
/**
* Includes view PHP or static file
*

0 comments on commit 38b1228

Please sign in to comment.