[4.0] Document: deprecate direct use of $_scripts and $_styleSheets

[Fedik]

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().

[mbabker]

This isn't right, you're changing a replace operation to an append operation.

[Fedik]

not really, I just changed how it looks like 😉
But result are the same: addScript() will do the same as array_replace(), if the $url exists, then it "replace", otherwise will "append"

[mbabker]

You're relying on a non-documented implementation detail to make this claim. The interface does not document whether the expected behavior is to overwrite an existing entry or raise an error, therefore if someone opted to subclass the Document class and throw an Exception if someone tries to register a URL twice, this assumption is broken.

At the end of the day, you're still changing the logic behind all of this. Right now it is crystal clear you are doing a replace operation. This refactoring looses clarity and proves some of the issues others have highlighted with this PR making an arbitrary assumption that the properties should only have getters.

[joeforjoomla]

Indeed if the direct use of public properties $_scripts and $_styleSheets is removed, at least there must be both getters and setters for those properties. Otherwise it won't be no longer possible to manipulate loaded scripts and stylesheets.

[Fedik]

The interface does not document whether the expected behavior is to overwrite an existing entry or raise an error

I think, if there no throws tag in a "method description" then there should not be Exception.
if someone throws an Exception then his implementation of the method are incorrect, no?
And duplication should be avoided by Document class, in one or another way, anyway.

[mbabker]

Not every throwing method has a documented @throws annotation, and actually a lot of interfaces in general fail to document "allowed" exception types (most of this isn't only related to Joomla, but since most people here don't touch PHP outside of it...). Likewise, not every method has @throws annotations if it doesn't catch exceptions from methods it calls (you then get into a debate whether the method should document only the exceptions it natively throws or whether it should document both those it throws and those that may be thrown by methods it calls, and at that point good luck keeping up with things). Therefore, relying on @throws annotations to indicate whether a method is allowed to throw an exception is not practical.

Either way, the point still stands there is no documented interface behavior, there is only the present de facto standard in the root class (which if this is what people want the method to do, that's fine, document it as the expected interface behavior (as that's exactly what an API specification is), but don't rely on "well this is what the root class does now so this is what will always happen"). Doc blocks are supposed to be more useful than "Adds a linked script to the page", but people are afraid to write useful documentation and instead view doc blocks as something that's just checking the block to pass PHPCS and write useless and doc blocks that are nothing more than a repetition of what the method signature says.

[wilsonge]

@Fedik just document in the method description the behaviour. I think the chances of people overriding this is fairly low and I'm happy to just make that description explicit rather than being an implementation detail.

[Fedik]

@wilsonge I have already updated it in the last commit 😉

[joeforjoomla]

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.

[laoneo]

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.

[mbabker]

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

[laoneo]

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

[mbabker]

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.

[mbabker]

(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)

[Fedik]

@mbabker this is what you meant? :)

[Fedik]

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

[joeforjoomla]

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.

[Fedik]

Yes that would be perfect.

I will add in the weekend

[Fedik]

drone said

fatal: refusing to merge unrelated histories

I not sure that it related to the PR

[Fedik]

hold on this, for now