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] Refactor JFTP error messages strings #31465

Merged
merged 2 commits into from
Dec 14, 2020

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Nov 23, 2020

Similar PR to #31456, but for JFTP strings in lib_joomla.ini

Currently, the language file lib_joomla.ini contains several strings which reference a PHP method. There are several issues with that:

  • The method name should not be translated, but is part of the language string.
  • The method is written in a format Classname: :Functionname (with a space between the : :). This isn't how a method is properly written.
  • The classname references the old JFTP class name, not the new namespaced one.
  • The same message exists multiple times, just for different functions. This is redundant work for TTs and not reuseable.

Summary of Changes

I've changed all strings starting with JLIB_CLIENT_ERROR_JFTP_ to a new format starting with JLIB_CLIENT_ERROR_FTP_ where the method name is passed using __METHOD__.
Eg:
JLIB_CLIENT_ERROR_JFTP_APPEND_BAD_RESPONSE="JFTP: :append: Bad response"
became
JLIB_CLIENT_ERROR_FTP_BAD_RESPONSE="%s: Bad response."

I've changed all calling places so the current method name (__METHOD__) is passed as a variable.

The amount of error strings went from 75 strings to 26.

Testing Instructions

You can try triggering an error by providing wrong credentials or so. But I think it's hard to trigger each error string.
So this should work with code review.

Actual result BEFORE applying this Pull Request

Error message would be for example
JFTP: :append: Bad response

Expected result AFTER applying this Pull Request

Error message would be for example
Joomla\CMS\Client\FtpClient::append: Bad response.

Documentation Changes Required

None

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Nov 23, 2020
@@ -1832,7 +1832,7 @@ protected function _passive()
// Make sure we have a connection to the server
if (!\is_resource($this->_conn))
{
Log::add(Text::_('JLIB_CLIENT_ERROR_JFTP_PASSIVE_CONNECT_PORT'), Log::WARNING, 'jerror');
Log::add(Text::sprintf('JLIB_CLIENT_ERROR_FTP_NO_CONNECT', __METHOD__), Log::WARNING, 'jerror');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one actually uses a wrong string before and after as the string expects host and port as variable.
As I don't know the proper error message here, I just left it the same it was before.

@ceford
Copy link
Contributor

ceford commented Nov 24, 2020

I have tested this item ✅ successfully on 6efae96

A lot to look at! I put all of the new strings in enqueueMessage statements in the site and admin templates, with dummy variables, just to check they render. They do. Can't test the actual FTP stuff.


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

@ghost
Copy link

ghost commented Nov 25, 2020

I have tested this item ✅ successfully on 6efae96


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

@alikon
Copy link
Contributor

alikon commented Nov 25, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 25, 2020
@HLeithner
Copy link
Member

Actually this is too late for j4, because removing language strings is a b/c break and have to be deprecated in 3.9.

So only way I see for this is to create only new JFTP strings (not sure if you did this) and deprecate the old one.

@infograf768
Copy link
Member

infograf768 commented Nov 25, 2020

@HLeithner
some strings have already been deleted in #31456 and replaced, as here, by new strings using different constants

@HLeithner
Copy link
Member

@Gostn don't know why you gave me a thumb down for just mentioning our b/c promise if you don't like it then create an issue and we can talk about it to remove it.

@brianteeman
Copy link
Contributor

brianteeman commented Nov 25, 2020 via email

@HLeithner
Copy link
Member

https://developer.joomla.org/development-strategy.html#backward_compatibility

6.1.6 Language keys
Changing or deleting a language key is considered a backwards compatibility break. Adding new ones is not. Substantially changing the meaning associated with a language key is a compatibility break. Rephrasing something for a more accurate description or proper en-GB grammar is not.

@HLeithner
Copy link
Member

if we don't care then we should remove this section from the b/c promise or extend it that they can be removed without notice in a major release. Changing the meaning should never be done anyway.

@brianteeman
Copy link
Contributor

Within a series I agree it's an amazing ssue. Between a series should be allowed as we can break b/c

@HLeithner
Copy link
Member

the point is only that the strings have not been deprecated, not that we can add new strings. Especially creating new strings doesn't hurt and removing the old strings in 5.0 with a deprecation notice in 4 is not a big deal.

@brianteeman
Copy link
Contributor

As JM and I already said there are - thousands of unused strings removed already bwithout deprecations

@HLeithner
Copy link
Member

That sounds like "we should break the rule even if we know that we breaking it" instead of fixing it....

@Bakual
Copy link
Contributor Author

Bakual commented Nov 25, 2020

The thing is, you can't really deprecated language strings.
You can add a comment on the line before, but only people actually looking into the INI file will see that. It will not be visible in Crowdin nor in PhpStorm. So it's not like deprecations in code where you actually get notified about it and thus the deprecation actually makes sense.

@brianteeman
Copy link
Contributor

The time to require that every string removed in j4 because it is not used has to be marked as deprecated in j3 (even though no one will see it) was before 29 Sep 2016 when the first PR to remove strings from J4 was made. That was a huge amount of work and you can be assured that I checked, checked and double checked that the PR would be accepted BEFORE even starting the work.

@wilsonge wilsonge merged commit 3dccd6c into joomla:4.0-dev Dec 14, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 14, 2020
@wilsonge
Copy link
Contributor

I think this is a big improvement on the old strings - much more user friendly. Thankyou!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 14, 2020
@Bakual Bakual deleted the 4_RefactorLibJFTP branch December 15, 2020 07:00
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