? ? Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
23 Jun 2019

Summary of Changes

Deprecate direct use of $_scripts, $_script and $_styleSheets, $_style.
Introduce Document::getScripts() Document::getStyleSheets() etc.

Testing Instructions

Apply the patch and make sure everything still works as before the patch.

Documentation Changes Required

If $_scripts, $_script and $_styleSheets, $_style mentioned somewhere, then it should be changed to Document::getScripts(), Document::getScriptDeclarations(), Document::getStyleSheets(), Document::getStyleDeclarations().

avatar Fedik Fedik - open - 23 Jun 2019
avatar Fedik Fedik - change - 23 Jun 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Jun 2019
Category Libraries
avatar Fedik Fedik - change - 23 Jun 2019
Labels Added: ?
avatar Fedik Fedik - change - 23 Jun 2019
Labels Added: ?
avatar joeforjoomla
joeforjoomla - comment - 23 Jun 2019

This PR is incomplete and not the equivalent of having access to $_scripts, $_script and $_styleSheets, $_style.

99% of cases we want to access $_scripts or $_styleSheets to manipulate or remove some file such as:
unset($doc->_scripts[$script]);
unset($doc->_styleSheets[$sheet]);

So if you want to make these properties as 'protected' in a future release and have a getter method, you should at least have even a related setter method.
getScriptDeclarations -> setScriptDeclarations
getScripts -> setScripts
...etc...

As a developer i still want to continue to remove a loaded script or stylesheet from the document as it's possible now.
There are plenty of extensions doing this out there.

avatar laoneo
laoneo - comment - 24 Jun 2019

If we start deprecating them, then please add the functions like getScript to the HTMLDocument class. We should not bind the generic Document class to HTML stuff.

avatar mbabker
mbabker - comment - 24 Jun 2019

Scripts and stylesheets are not unique to HTML output only. You can use stylesheets within XML documents, as an example.

avatar laoneo
laoneo - comment - 24 Jun 2019

But then it would be better to make a base class for script based documents.

avatar mbabker
mbabker - comment - 24 Jun 2019

It'd be better to use the decorator pattern in all honesty because there are a number of properties and methods that do not apply to all output formats, but then you are forcing a lot of downstream code to introduce instanceof Foo or method_exists() checks for no real reason (and then you have to justify what behaviors should exist in something like the raw document, which doesn't align with a "proper" response format). The time for introducing that type of distinguishing logic was 10 years ago, the ship has sailed on building a Document API where someone makes an arbitrary decision to say that the scripts and stylesheets properties (as examples) will never be available in raw or JSON context.

avatar mbabker
mbabker - comment - 24 Jun 2019

(Or, just drop everything but HTML from core like I've lightly suggested a number of times because nobody wants to make any non-HTML outputs proper first class citizens and everyone just assumes Joomla will always print HTML or bypasses core if they do non-HTML to the point where it doesn't matter how broken core is)

avatar Fedik
Fedik - comment - 25 Jun 2019

you should at least have even a related setter method.
getScriptDeclarations -> setScriptDeclarations
getScripts -> setScripts etc

I can add removeScript, removeStyleshet etc
All scripts/styleshets should be added via addScript() addStyleShet() etc

avatar joeforjoomla
joeforjoomla - comment - 25 Jun 2019

you should at least have even a related setter method.
getScriptDeclarations -> setScriptDeclarations
getScripts -> setScripts etc

I can add removeScript, removeStyleshet etc
All scripts/styleshets should be added via addScript() addStyleShet() etc

Yes that would be perfect. This is something that has always been missing in the Document API and in past years developers have been forced to use unset on the public properties.

avatar Fedik
Fedik - comment - 27 Jun 2019

Yes that would be perfect.

I will add in the weekend

avatar Fedik
Fedik - comment - 28 Jun 2019

drone said

fatal: refusing to merge unrelated histories

I not sure that it related to the PR

avatar Fedik
Fedik - comment - 9 Jul 2019

hold on this, for now

avatar Fedik Fedik - change - 9 Jul 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-07-09 10:56:43
Closed_By Fedik
avatar Fedik Fedik - close - 9 Jul 2019

Add a Comment

Login with GitHub to post a comment