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

[3.9] Make the php security only warning more generic #21987

Merged
merged 3 commits into from Sep 18, 2018

Conversation

Projects
None yet
9 participants
@StefanSTS
Contributor

StefanSTS commented Sep 4, 2018

Pull Request for Issue #21969 .

Summary of Changes

Changed language variable PLG_QUICKICON_PHPVERSIONCHECK_SECURITY_ONLY.
Removed the recommendation for PHP 7.x, called it more generic "newer version".
Short sentences with the necessary information.

Testing Instructions

Replace the ini file and check the warning in Joomla control panel if you have PHP 7.0.x.
Quickitem phpversioncheck needs to be activated.

Expected result

Clear warning message.

Documentation Changes Required

none

Update en-GB.plg_quickicon_phpversioncheck.ini
Removed the recommendation for PHP 7.x, called it more generic.
Short sentences with the necessary information.

@StefanSTS StefanSTS requested a review from brianteeman as a code owner Sep 4, 2018

@zero-24 zero-24 added this to the Joomla 3.9.0 milestone Sep 4, 2018

@zero-24 zero-24 changed the title from Update en-GB.plg_quickicon_phpversioncheck.ini to [3.9] Make the php security only warning more generic Sep 4, 2018

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 4, 2018

The only thing I would change with the original string would have been to remove this part
(PHP 7.x is recommended)

I don't see the purpose of the other changes - which are a complete rewrite adding work for the translators

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 4, 2018

I am fine with any changes as long as it is formally right. Please go ahead.
My version is shorter, has three short sentences with the most important information. I like to keep it short and precise.
The grammar in the current text is not really up to the mark, so I took the chance to change that. The translators have to touch the text anyway and I don't see too much work for them added. Looking at the German translation the Grammar is already correct.
If we correct it one way or the other isn't really a big deal. If the user understands it it's fine. We should just do it.

@Quy

This comment has been minimized.

Contributor

Quy commented Sep 4, 2018

I have tested this item successfully on 4e97a01

I like the new version. Short and simple.


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

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 5, 2018

Unfortunately translators sometimes do not translate but replace english words by words of their own language, which results in difficult sentences. In german I will suggest to use even a shorter Information:

"Your PHP version, %1$s, will reach end of support on %2$s. We recommend upgrading to a newer PHP version soon to keep your Joomla installation secure. Please contact your host for upgrade instructions."


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

This comment has been minimized.

Contributor

StefanSTS commented Sep 5, 2018

Sounds even better to me, chmst.
The variable for "unsupported PHP version" could use a trim too. But I wanted to keep the hurdles for closing this issue as tiny as possible.
Can we agree on chmst's suggestion?

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 6, 2018

@StefanSTS commit your prefered version and we will see what happens.

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 11, 2018

@chmst I hope I changed it to your version at the right place now. Do we need any tests or something more like that?

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 11, 2018

I have tested this item successfully on 552783d


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

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 11, 2018

Thank you. I am no native speaker but looks good for me.


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

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Sep 11, 2018

@Quy can you please retest?

@tecpromotion

This comment has been minimized.

Contributor

tecpromotion commented Sep 12, 2018

I have tested this item successfully on 552783d

Tested successfully.


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.9.0 milestone Sep 12, 2018

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Sep 12, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 12, 2018

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 12, 2018

i still dont like this change as it has changed the meaning of the message and i stick by my earlier comment

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

@brianteeman In my opinion everything is still in the message except for "faster php".
The former message states three times that the security will be a problem.
"is only receiving security fixes"
"your PHP version will soon no longer be supported"
" before it reaches end of support on %2$s"
This is all covered in:
Your PHP version, %1$s, will reach end of support on %2$s. .... to keep your Joomla installation secure."
I don't see any reason to make the user read so much text which will make some users stop in the middle and disregard the message. What happens to long messages often.

@mbabker

This comment has been minimized.

Member

mbabker commented Sep 12, 2018

The "faster and more secure" part is a bit of marketing text, not much different than what https://github.com/WordPress/servehappy is proposing for that platform. It's also explaining a benefit to the user to complete the action, not just "oh the software powering your site is no longer supported".

Personally I'd only remove the "PHP 7.x is recommended" bit, in hindsight it wasn't the best of ideas to specify a version in the string (we should've made it a sprintf and dynamically pushed it in).

One thing we need to be careful of, no matter how many times it is suggested these nag messages be revised or removed outright, is making the message too concise. We aren't linking to any external resources explaining the alert or how to address it, so we need to ensure that the message is crystal clear with the text we give users. There is a part of me that feels like this change might be making the message too concise.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 12, 2018

The proposed message does not say anywhere that the version of php is only receiving security fixes AND very importantly it makes no mention of the fact that this is a php project issue and not a joomla one.

I am 100% in favour of shorter messages but we can't go so short as to remove the important meaning of the message.

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

How does that sound?
The PHP project is supporting your PHP version, %1$s, only until %2$s with security fixes. We recommend upgrading to a newer PHP version soon to keep your Joomla installation secure. Please contact your host for upgrade instructions.

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

Giving warning and error messages a number or some alphanumeric value might be a good idea. Could be easier to find them on a search engine in other languages and someone might even consider to make a list of them with possible solutions. Might be worth to open an issue for that.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 12, 2018

Sorry but it still doesnt work for me. It doesnt stress that it is only getting security fixes and not any bug fixes. We did spend a lot if time writing the original string :) We just made a mistake including the version number

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

To make it a generic message, you have to remove the "faster" too. One of the next versions might not be faster, it might have more features and gets slower. Except you want to keep that advertising aspect. So give us your best shot @brianteeman and we can finalise this before we spend more time changing the text than you spend on creating it. ;-)

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 12, 2018

I already did #21987 (comment)

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

Keep the faster? That might be false advertising in future versions. That cannot come from my pull request.

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 12, 2018

I accept the arguments, so let us come back to the original content (without version number).
Maybe a simple linebreak after every sentence could do the trick and make reading and understanding easier, this was the intention for this PR.

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

So another try:
Your PHP version, %1$s, is only receiving security fixes from the PHP project at this time. This means your PHP version will soon no longer be supported. We recommend planning to upgrade to a newer PHP version before it reaches end of support on %2$s. Joomla will be faster and more secure if you upgrade to a newer PHP version. Please contact your host for upgrade instructions.

Of course with my strongest protest about the faster. But lets end it.
Is that fine like this?

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 12, 2018

The file is changed to the mentioned version above, please test if that is fine now.
Thanks.

@StefanSTS

This comment has been minimized.

Contributor

StefanSTS commented Sep 14, 2018

If there are any more steps I have to do to finalize this request, please advise. I see all green markers so I hope with @brianteeman 's go, this can be merged.

@mbabker mbabker added this to the Joomla 3.9.0 milestone Sep 15, 2018

@brianteeman

approve language change

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Sep 16, 2018

I have tested this item successfully on 80745bd


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

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Sep 16, 2018

@chmst can you please retest?

@chmst

This comment has been minimized.

Contributor

chmst commented Sep 16, 2018

I have tested this item successfully on 80745bd


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

@mbabker mbabker merged commit 03e5e4a into joomla:staging Sep 18, 2018

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 18, 2018

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