Skip to content

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Sep 21, 2020

Pull Request for Issue #30499 .

Summary of Changes

Log the from version on Joomla update event on the Actionlogs

Testing Instructions

apply pr
joomlaupdate -> options
change to Custom URL
and put this manifest https://ci.joomla.org/artifacts/joomla/joomla-cms/staging/30714/downloads/35635/pr_list.xml
is the one available on bottom near Download — Prebuilt packages are available for download.

Actual result BEFORE applying this Pull Request

information about from version not available

Expected result AFTER applying this Pull Request

information about from version available
image

Documentation Changes Required

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Sep 21, 2020
@alikon alikon changed the title [wip] - show from to when update [3.9] Log the from version on actionlogs when Joomla update Sep 21, 2020
@alikon alikon marked this pull request as ready for review September 21, 2020 13:47
@richard67
Copy link
Member

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

@alikon
Copy link
Contributor Author

alikon commented Sep 21, 2020 via email

@richard67
Copy link
Member

I have tested this item ✅ successfully on c3c8480


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30714.

1 similar comment
@ChristineWk
Copy link

I have tested this item ✅ successfully on c3c8480


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30714.

@Quy
Copy link
Contributor

Quy commented Sep 21, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30714.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 21, 2020
@SharkyKZ
Copy link
Contributor

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

@HLeithner
Copy link
Member

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?

@SharkyKZ
Copy link
Contributor

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.

@alikon
Copy link
Contributor Author

alikon commented Sep 22, 2020

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

@richard67
Copy link
Member

richard67 commented Sep 22, 2020

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.

@HLeithner
Copy link
Member

Yes please add ="unknown"

@alikon
Copy link
Contributor Author

alikon commented Sep 23, 2020

added

@HLeithner
Copy link
Member

Thanks, plz 2 tests and can be merged

@HLeithner
Copy link
Member

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

@alikon alikon removed the RTC This Pull Request is Ready To Commit label Sep 23, 2020
@richard67
Copy link
Member

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 23, 2020
@richard67
Copy link
Member

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

@SharkyKZ
Copy link
Contributor

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.

@richard67
Copy link
Member

@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?

@HLeithner
Copy link
Member

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

@richard67
Copy link
Member

@HLeithner

no you don't need to create a plugin

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

@richard67
Copy link
Member

@alikon Could you change it to

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

?

@HLeithner
Copy link
Member

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

wrong wording sorry

@richard67
Copy link
Member

wrong wording sorry

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

@richard67
Copy link
Member

richard67 commented Sep 23, 2020

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

@richard67
Copy link
Member

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"

  1. 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"


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30714.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants