@alikon It seems that it needs to apply the patch of this PR before doing the update to that package which you have linked.

Yes i forgot that, I'll add it

Il lun 21 set 2020, 16:05 Richard Fath notifications@github.com ha
scritto:

@alikon https://github.com/alikon It seems that it needs to apply the
patch of this PR before doing the update to that package which you have
linked.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30714 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABMLMKAXH7KBVD463FQPOLSG5MR5ANCNFSM4RUKTPUA
.

I have tested this item ✅ successfully on c3c8480

I have tested this item ✅ successfully on c3c8480

RTC

This could be a B/C break if onJoomlaAfterUpdate is triggered by 3rd party extension.

Why should onJoomlaAfterUpdate be triggered by a 3rd party extension? I mean we have no way to protect events but do you think a 3rd party extension triggers this?

Or do you mean the plugin should add a default value for the $oldVersion parameter?

Given that this event was recently added, is undocumented and has a very limited use case, I don't think anyone is actually using it. But if you want to be really careful, adding a default value would be a sensible thing to do.

did you mean someting like public function onJoomlaAfterUpdate($oldVersion = 'unknown') ?

Maybe null or empty string as default, and in the code use a JTEXT for 'unknown' if the parameter is empty, so it can be translated?

On the other hand, that seems to be a bit over-engineered for something which should never happen.

So I'd be ok with anything, no default like now, or hard-coded like suggested above.

Yes please add ="unknown"

added

Thanks, plz 2 tests and can be merged

plz also with a plugin that triggers the event without parameter.

Wouldn't is be better to use an empty string as default value for $oldVersion and use JText::_('JLIB_UNKNOWN') if $oldVersion is empty?

@HLeithner I don't have such plugin so I can't test.

Wouldn't is be better to use an empty string as default value for $oldVersion and use JText::_('JLIB_UNKNOWN') if $oldVersion is empty?

IMO should be null. But realistically it makes no difference. It's only used internally by this plugin.

@HLeithner Shouldn't a code review be sufficient for the default value part of this PR? Does it really need to write a plugin to test this?

if you use null (without language) you will end in a broken text string "...updated Joomla from to 3.9.22"

Actually while testing you see this broken string, so I would suggest to catch this case better and change the code from

$oldVersion='unknown' to $oldVersion=null and test on if empty($oldVersion) and set the unkown version string...

@richard67 no you don't need to create a plugin you only have to trigger the event for example in a template:

JFactory::getApplication()->triggerEvent('onJoomlaAfterUpdate');

or

JFactory::getApplication()->triggerEvent('onJoomlaAfterUpdate', array(null));
JFactory::getApplication()->triggerEvent('onJoomlaAfterUpdate', array(''));
JFactory::getApplication()->triggerEvent('onJoomlaAfterUpdate', array('5.0.0));

@HLeithner

no you don't need to create a plugin

Well, you wrote "plz also with a plugin that triggers the event without parameter." ?

@alikon Could you change it to

	public function onJoomlaAfterUpdate($oldVersion = null)
	{
...		
		if (empty($oldVersion))
		{			
			$oldVersion = JText::_('JLIB_UNKNOWN');	
		}

?

Well, you wrote "plz also with a plugin that triggers the event without parameter." ?

wrong wording sorry

wrong wording sorry

Now you say that? I almost have my plugin ready ? . (joking)

I've updated the custom update URL in the testing instructions to the one of the new build.

I have tested this item ✅ successfully on ee6b10f

1. Real test as described:

Result: "User whoeveriamdoesnotmatter updated Joomla from 3.9.22-dev to 3.9.22-dev+pr.30714"

2. Following in index.php of the isis template:

$app->triggerEvent('onJoomlaAfterUpdate');
$app->triggerEvent('onJoomlaAfterUpdate', array(null));
$app->triggerEvent('onJoomlaAfterUpdate', array(''));
$app->triggerEvent('onJoomlaAfterUpdate', array('5.0.0'));

Result: 3 times "User whoeveriamdoesnotmatter updated Joomla from Unknown to 3.9.22-dev+pr.30714", one time "User whoeveriamdoesnotmatter updated Joomla from 5.0.0 to 3.9.22-dev+pr.30714"