PR-4.3-dev ? Pending

User tests: Successful: Unsuccessful:

avatar Ruud68
Ruud68
27 May 2021

Pull Request for Issue #34228 .

Summary of Changes

With this option extension developers can decide on their own use case if they want to use full js / css files or the minified versions without being forced to switch the entire site into debug mode (global option)

Testing Instructions

add a script to your extensions webpage

HTMLHelper::_('script', 'media/system/js/showon.js', ['debugMode' => true]);

Actual result BEFORE applying this Pull Request

minified version is loaded
<script src="/media/system/js/showon.min.js"></script>

Expected result AFTER applying this Pull Request

full version is loaded
<script src="/media/system/js/showon.js"></script>

Documentation Changes Required

? don't think there is documentation on this specific method

avatar Ruud68 Ruud68 - open - 27 May 2021
avatar Ruud68 Ruud68 - change - 27 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 May 2021
Category Libraries
avatar Ruud68 Ruud68 - change - 27 May 2021
The description was changed
avatar Ruud68 Ruud68 - edited - 27 May 2021
avatar Ruud68 Ruud68 - change - 27 May 2021
Title
add debugMode option for better script and css handling
add debugMode option for more control over script and css loading
avatar Ruud68 Ruud68 - edited - 27 May 2021
avatar Ruud68 Ruud68 - change - 28 May 2021
Title
add debugMode option for more control over script and css loading
[4.0] add debugMode option for more control over script and css loading
avatar Ruud68 Ruud68 - edited - 28 May 2021
avatar Fedik
Fedik - comment - 29 May 2021

I not sure here.
It will be confusing to have 2 option $detectDebug and $debugMode

avatar Ruud68
Ruud68 - comment - 29 May 2021

I not sure here.
It will be confusing to have 2 option $detectDebug and $debugMode

detectDebug is for me the confusing one: it doesn't detect debug but what it does is when set force minified versions even when debug is enabled.

debugMode is exactly what it is: a toggle set to debug ( = full version of the scripts / css) even when JDEBUG is set to no.
$debugMode is already used in the function, what I do in this PR is make it an option to the function instead of set it in the function to default value false.

As an extension developer when debugging I want to debug as close to 'production' as possible. Using JDEBUG changes not only the behavior of my extension but of all extensions / template / etc. and Joomla.
This option allows me to only change my extension for debugging where the rest remains as it normally is.

avatar Fedik
Fedik - comment - 29 May 2021

detectDebug is for me the confusing one: it doesn't detect debug but what it does is when set force minified versions even when debug is enabled.

Maybe better if we can fix this part somehow, to keep original file name. Instead of introducing another option, or?

Using JDEBUG changes not only the behavior of my extension but of all extensions / template / etc. and Joomla.

yeah, true

avatar Ruud68
Ruud68 - comment - 29 May 2021

Maybe better if we can fix this part somehow, to keep original file name. Instead of introducing another option, or?

For me that would be a B/C break. I learned these variables by debugging through the code as these are nowhere (nowhere I could find them) described, but it is possible that these are actively used by other devs?

adding $debugMode as variable (this PR) is fully B/C and gives control over all possible combinations

avatar Ruud68
Ruud68 - comment - 21 Sep 2021

Hi, it has been months already. Can somebody please advice or make a decision?

avatar Fedik
Fedik - comment - 21 Sep 2021

Unfortunately it will not go to J4 as it is, because signature change of includeRelativeFiles().
Hm, need to think something else.

avatar Ruud68
Ruud68 - comment - 22 Sep 2021

Unfortunately it will not go to J4 as it is, because signature change of includeRelativeFiles().

J4 as in 4.0.x or J4.x?

avatar Fedik
Fedik - comment - 22 Sep 2021

for 4.0.x for sure,
hm, maybe for J4.x, but still risky to me :)

avatar Ruud68
Ruud68 - comment - 22 Sep 2021

Thanks, but who decides on this? Is this somewhere on the agenda to be looked at?
Spent a lot of time on this to troubleshoot and come up with an (non B/C breaking) improvement.

avatar Fedik
Fedik - comment - 22 Sep 2021

Cannot say,
I guess first it need to be tested ;)

avatar alikon
alikon - comment - 22 Sep 2021

It needs at least 2 successfull tests, then it will be marked as Ready To Commit, and after is up to Release Leader to decide upon

avatar Ruud68
Ruud68 - comment - 23 Sep 2021

It needs at least 2 successfull tests, then it will be marked as Ready To Commit, and after is up to Release Leader to decide upon

Changes like these will never make it into core as this is a pretty specific (developer) use case that other developers have probably worked around for a long time (as I did).
furthermore: this is really a nobrainer and the impact can be determined by a simple code review: you can see that it doesn't break B/C.

For other changes I did I spent a lot of time rallying for testers amongst my clients, this will not work for these kind of changes....

Fingers crossed that another dev will run into this and takes the time to look for a solution other then to build a work around.
just my 2cts.

Will keep this PR open for a while, see what happens.
Not a big fan of long time open PR's as every time I see them I wonder 'why did I bother' :) not so motivating #lol

avatar Fedik
Fedik - comment - 23 Sep 2021

you can see that it doesn't break B/C.

Unfortunately it is, changing method signature lead to fatal error on PHP 8 https://php.watch/versions/8.0/lsp-errors ,
and I am sure there is a people who extends HTMLHelper

Will keep this PR open for a while, see what happens.

yes please

avatar Ruud68
Ruud68 - comment - 23 Sep 2021

Unfortunately it is, changing method signature lead to fatal error on PHP 8 https://php.watch/versions/8.0/lsp-errors ,
and I am sure there is a people who extends HTMLHelper

when extending HtmlHelper and in that extended class explicitly extending the protected includeRelativeFiles, php 7 will give a warning, php 8 an error.
But IMO that is part of maintaining your own code, that should not stop progress on the Joomla side because if it did, Joomla would still be on php5 probably :)

avatar Fedik
Fedik - comment - 25 Sep 2021

On quick test it works good.
Please also update this part

// Get the file path
$file = HTMLHelper::_(
$type,
$path,
[
'pathOnly' => true,
'relative' => !$this->isPathAbsolute($path),
]
);

To use this option:

// Get the file path
$file = HTMLHelper::_(
	$type,
	$path,
	[
		'pathOnly'  => true,
		'relative'  => !$this->isPathAbsolute($path),
		'debugMode' => $this->getOption('debug', false),
	]
);
avatar Ruud68 Ruud68 - change - 27 Sep 2021
Labels Added: ?
avatar Ruud68
Ruud68 - comment - 27 Sep 2021

Thanks @Fedik , debugModel added to webassetitem

avatar HLeithner
HLeithner - comment - 27 Jun 2022

This pull request has automatically rebased to 4.2-dev.

avatar joomla-bot
joomla-bot - comment - 27 Jun 2022

This pull requests has been automatically converted to the PSR-12 coding standard.

avatar rdeutz rdeutz - change - 21 Oct 2022
Title
[4.0] add debugMode option for more control over script and css loading
Add debugMode option for more control over script and css loading
avatar rdeutz rdeutz - edited - 21 Oct 2022
avatar rdeutz rdeutz - change - 21 Oct 2022
Labels Added: PR-4.3-dev ?
Removed: ?
avatar bembelimen bembelimen - change - 23 Oct 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-10-23 08:32:55
Closed_By bembelimen
avatar bembelimen
bembelimen - comment - 23 Oct 2022

Hello @Ruud68 we like this idea, but as we have the asset manager now, we should implement this e.g. in the joomla.asset.json to set debug for certain files individually not using the HTMLHelper anymore. Additionally it is a B/C break when we merge it into 4.3.
Probably you're willing to give it a try directly in the Webasset Manager for Joomla! 5?

avatar bembelimen bembelimen - close - 23 Oct 2022
avatar Ruud68
Ruud68 - comment - 23 Oct 2022

Hi @bembelimen thanks for taking the time to review and appreciate this contribution. I appreciate it!
I closed this, and other PR's I did for Joomla, a long time ago in my mind. Lessons learned :)

Add a Comment

Login with GitHub to post a comment