Skip to content

Revert "[4.1] Add server timing response header in debug mode (#36231)" - #36840

Closed
bembelimen wants to merge 1 commit into
joomla:4.1-devfrom
bembelimen:4.1/reverts/36231
Closed

Revert "[4.1] Add server timing response header in debug mode (#36231)"#36840
bembelimen wants to merge 1 commit into
joomla:4.1-devfrom
bembelimen:4.1/reverts/36231

Conversation

@bembelimen

Copy link
Copy Markdown
Contributor

This reverts commit 360409f.

Summary of Changes

Sorry, was to fast in merging. We should for new events only go with the new event system. So probably @laoneo you could just do the same PR with the new event system?

Sorry for the trouble.

@Fedik

Fedik commented Jan 25, 2022

Copy link
Copy Markdown
Member

Also can just change onBeforeRespond to onAfterRender, that triggered almost before response.

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

I do not agree. It uses the dispatcher which is new way. So should an empty event being introduced here or what?

@Fedik

Fedik commented Jan 25, 2022

Copy link
Copy Markdown
Member

So should an empty event being introduced here or what?

We have RFC for versioning joomla/rfc#29 (though it still RFC)

Changes that do not change signatures of functions or methods of the public API and do not add new ones can go
into a patch release.

New Event add a new public API, so it a new feature 😉
And new feature should not go in RC.

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

@Fedik please read the sentence again. This is not a signature change! And it is a about patch releases and does not mention in any way RC.

@Fedik

Fedik commented Jan 25, 2022

Copy link
Copy Markdown
Member

This is not a signature change!

Correct, but a new public API.

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

Also can just change onBeforeRespond to onAfterRender, that triggered almost before response.

Actually I do not care which event is used. If there is an existing one, then this pr can change to it and revert the "new" event. But reverting a whole pr which adds some debug functionality is just ridiculous.

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

This is not a signature change!

Correct, but a new public API.

Then make a pr which uses your suggested event and all is good. I will immediately test it.

@HLeithner

Copy link
Copy Markdown
Member

Actually the original PR fixes a missing event (onBeforeRespond) which already exists in the abstracted class WebApplication (so it's not needed by allon to create the right event him self) but is missing in all abstracted and overridden classes. Beside this the event is also not triggered in the caching plugin and so would not add performance matrix when the page comes from the cache.

So I revert my opinion on reverting this PR since the event already exists, only the PR doesn't do what would be expected if the page is full cached because of the missing onBeforeRespond in the caching plugin.

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

Keep in mind, this is only something which should work in debug mode and hopefully the caching plugin is not running when debugging.

@HLeithner

HLeithner commented Jan 25, 2022

Copy link
Copy Markdown
Member

Keep in mind, this is only something which should work in debug mode and hopefully the caching plugin is not running when debugging.

actually it does, looking at the code it also triggers the onAfterResponse event only in debugmode:
https://github.com/joomla/joomla-cms/blob/4.1-dev/plugins/system/cache/cache.php#L147-L155

@laoneo

laoneo commented Jan 25, 2022

Copy link
Copy Markdown
Member

Wouldn't expect that...

@bembelimen bembelimen closed this Jan 25, 2022
@bembelimen
bembelimen deleted the 4.1/reverts/36231 branch March 15, 2024 00:08
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.

5 participants