Skip to content

[3.10] Clear buffered output at all levels#30786

Merged
richard67 merged 7 commits intojoomla:3.10-devfrom
zero-24:redirect_plugin
Jan 19, 2021
Merged

[3.10] Clear buffered output at all levels#30786
richard67 merged 7 commits intojoomla:3.10-devfrom
zero-24:redirect_plugin

Conversation

@zero-24
Copy link
Contributor

@zero-24 zero-24 commented Sep 27, 2020

Pull Request for Issue #18088

Summary of Changes

Add back the check to clear the buffered output. cc @SharkyKZ

Testing Instructions

Enter non-existing url in browser to trigger 404 page.

Actual result BEFORE applying this Pull Request

Default template error page should be displayed, as usual.

Expected result AFTER applying this Pull Request

Default template error page should be displayed, as usual.

Documentation Changes Required

None

@ethernidee
Copy link
Contributor

The following code:

// Clear buffered output at all levels
$callerFunction = static::getCallerFunctionName();

if ($callerFunction === false)
{
  while (ob_get_level())
  {
    ob_end_clean();
  }
}

should be replaced with:

// Clear buffered output at all levels
while (ob_get_level())
{
  ob_end_clean();
}

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 27, 2020

Ok will do when i'm back at my PC.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 27, 2020

Thanks @ethernidee this has just been commited to this PR.

@particthistle
Copy link
Member

I have tested this item ✅ successfully on 0fda10a

Default template error page should be displayed, as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30786.

@SharkyKZ
Copy link
Contributor

Tests are failing.

@ethernidee
Copy link
Contributor

  1. JErrorPageTest::testEnsureTheErrorPageIsCorrectlyRendered
    Failed asserting that false matches expected '<title>500 - Testing JErrorPage::render() with RuntimeException</title>Testing JErrorPage::render() with RuntimeException'.

/drone/src/tests/unit/suites/libraries/cms/error/JErrorPageTest.php:79

  1. JErrorPageTest::testEnsureTheErrorPageIsCorrectlyRenderedWithThrowables
    Failed asserting that false matches expected '<title>500 - Testing JErrorPage::render() with PHP 7 Error</title>Testing JErrorPage::render() with PHP 7 Error'.

/drone/src/tests/unit/suites/libraries/cms/error/JErrorPageTest.php:123


Such things cannot be tested if buffer is cleared properly. The tests are invalid, imho.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 29, 2020

Such things cannot be tested if buffer is cleared properly. The tests are invalid, imho.

What is the point of clearing the buffer there anyway?

@ethernidee
Copy link
Contributor

Imagine you render Joomla module. First level of buffer capturing is on. Then in module template you write ob_start() for internal subwidget. If any error happens, Joomla will clear second level buffer and leave top of the default template rendered + will add error page html. This is totally wrong. All buffer levels should be cleared before rewriting the whole response, whether it's json or html response.

I faced such situations on working sites and was really surprised.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 29, 2020

Makes sense, thanks

@zero-24
Copy link
Contributor Author

zero-24 commented Oct 3, 2020

The latest commits make sure the test suite passes as it starts the output buffer right after we cleaded it again. Else that thing will fail as the output puffes is clreandend but not restarted.

@ethernidee
Copy link
Contributor

Excellent solution!

@richard67 richard67 merged commit eb39d1e into joomla:3.10-dev Jan 19, 2021
@richard67
Copy link
Member

Thanks!

@zero-24 zero-24 deleted the redirect_plugin branch January 19, 2021 14:40
@@ -176,7 +182,7 @@ public static function render($error)
protected static function getCallerFunctionName()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zero-24 I think this function can be removed now - it's not longer being called

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be used as its part of the API right? It can be removed with 4 i guess right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a protected function and introduced in 3.10 - its only been in alpha's so far?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok will do a PR to get rid of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR done: #32175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants