Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] HTMLHelper script(). 0 - Call to a member function addScript() on null #25669

Closed
wants to merge 2 commits into from

Conversation

@ReLater
Copy link
Contributor

commented Jul 20, 2019

Testing Instructions

  • Deactivate system cache plugin.
  • Add the following code lines in plugins/system/cache/cache.php in method onAfterRoute()
	public function onAfterRoute()
	{
		$file = 'com_banners/admin-banner-edit.min.js';
	
		Joomla\CMS\HTML\HTMLHelper::_('script', $file,
		 array('version' => 'auto', 'relative' => true)
		);

		return;

...and so on...
  • Activate the plugin. => Error.

  • Deactivate plugin by renaming folder plugins/system/cachexxxx/

  • Apply patch.

  • Rename back folder plugins/system/cache/ to activate it.

  • Reload backend ==> No error.

Expected result

  • No error and $file loaded.

Actual result

Error:

0 - Call to a member function addScript() on null

1 | ( ) | JROOT/libraries/src/HTML/HTMLHelper.php:736
2 | Joomla\CMS\HTML\HTMLHelper::script()
@ReLater

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

Identical test with stylesheet

		$file = 'com_associations/sidebyside.css';
	
		Joomla\CMS\HTML\HTMLHelper::_('stylesheet', $file,
		 array('version' => 'auto', 'relative' => true)
		);

		return;
@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

The problem is that you're using onAfterRoute to add scripts. The document hasn't been created at this point yet. Use a later event.

@ReLater

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

The problem is that you're using onAfterRoute to add scripts. The document hasn't been created at this point yet. Use a later event.

That worked in Joomla 3 without any problems over years. If that behavior was changed in Joomla 4 then that's a harsh B\C break.

Test the same scenario in Joomla 3.

Or compare the code in Joomla 3.
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/HTML/HTMLHelper.php#L651
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/HTML/HTMLHelper.php#L731

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

@SharkyKZ it must still be perfectly lecit to add scripts or stylesheets in the onAfterRoute event, why not?

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

It not a bug, it feature 😉

That worked in Joomla 3 without any problems over years.

It worked, but It was incorrect even for Joomla 3, and that made many bugs because the document may be incorrectly instantiated, because route not parsed at that point of time.

Try on afterRoute at least, or afterDispatch

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

@Fedik pay attention, @ReLater is already using and talking about onAfterRoute, so route is parsed

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

Route is parsed but the document hasn't been created yet. Use onAfterDispatch or later event to manipulate the document.

Furthermore, your code doesn't even check for document type. You should always check for document type. Otherwise errors will occur on other document types.

I think this PR can be closed.

@ReLater

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

I think this PR can be closed.

Not until I have a clear answer why this grave B\C break is a "feature". I'm still convinced that it is a bug.

BTW: I know why I use it in onAfterRoute and not (again) later and next time (again) later...

@Fedik pay attention, @ReLater is already using and talking about onAfterRoute, so route is parsed

That's correct! Thus @Fedik 's comment is irrelevant.

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

@ReLater is right

This is a hard B\C break that could potentially affect hundreds of plugins out there.

Moreover you can still use a deprecated code in the 3.x fashion to create a document object in the onAfterRoute event:

$doc = Joomla\CMS\Factory::getDocument();

Given that this is a deeper change it should be at least well documented to update plugins accordingly.

@mbabker

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

You can argue the B/C semantics all day long, the fact of the matter is that instantiating the Document before it is intended to be can and does cause side effects if not handled properly. So it is not right, ever, for a plugin to be loading assets to the Document before the component is dispatched, and the fact it might have worked in 3.x is moreso based on flawed architecture and not being a core feature that a plugin can force Document instantiation before the request is routed and leave the application in an unexpected state (i.e. pushing assets on a JSON request into the Document before the app creates it at the intended time is going to result in the HtmlDocument being used instead of the JsonDocument, this is why extension developers have a suite of hacks to short circuit core when a non-HTML response format is intended).

@mbabker

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Also, it is perfectly valid for the Document to not be instantiated before onAfterRoute is emitted. Plugins can still hook the system and make changes to the environment before the Document is created (using custom GET var or session var to force a format is one intricate example I can think of). So the answer is not moving Document instantiation and there’s an entire other issue bashing out why so much of the core request-response workflow is flawed and needs redesigning.

@ReLater

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

Thanks!

Added to documentation: https://docs.joomla.org/Plugin/Events/System#onAfterRoute

@ReLater ReLater closed this Jul 22, 2019

@ReLater ReLater deleted the ReLater:patch-1 branch Jul 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.