What is the reason for changing this? I see several potential issues with this change but will reserve commenting on those until I understand the reason for the change

Well, the type="number" field exists exactly for this scenarios, ie, only accept values (client side and server side) number values (or a range of allowed numbers). That is exactly what this port fields can accept (like session timeout field).
So in my opinion is a matter of using the right type of field for this port fields.

Also, TCP ports can only be from numbers from 1 to 65536.

My apologies. I thought this would give us a big popup select and not allow direct input - need more coffee

😉

It all works fine. The only thing I am not sure about is setting the default values. This is a change in behaviour as previously if you didnt have a proxy setup (for example) then no port would be saved now the default port is saved.

yeah not sure of that also.

Personally I would use either a default value or a hint. Using both doesn't make that much sense to me. Also when you define a default value in the XML, you should make sure the same default value is also set in the correspondig PHP code.

In this cases here where there was no default specified I would just use the hint attribute, but then not simply show the default value (since it can be confused with an already saved value) but show a comment text like "default port is 1234" or so. But that's me.

agree with @Bakual , using a hint/comment would be better instead of the default value filled.
Great improvement, thank you @andrepereiradasilva

Can you add the same changes to the three views in com_joomlaupdate that also have FTP Port fields please

Field changed from text to number. Limit changed. Tootip visible when values are greater than 65535 or not number.
But one thing I notice in both FTP and SMTP Port field that it allows some invalid characters while typing:

@hardiktailored What browser are you using, specifically is it one that supports the native number input type? I've pointed out some problems in the past (I can't find the issue with those notes anymore sadly) with some issues with the polyfill for that field type and you may be hitting another unreported issue with it.

@mbabker I also tried what @hardiktailored tested and I see that it is no problem to enter something like "21e+-" into e.g. the FTP Port field. The browser I'm using is Firefox 47.0.1 btw.

The field is marked with red borders in order to signal that something is wrong with the data I've entered, but I can go to the next field and even send the form to the server. But when I look into the data sent via POST to the server I see that instead of my data an empty string is sent to the server (for this field).

So I'm not sure whether we can expect from current browsers that they refuse to accept characters that are not allowed for numbers. Instead they seem to accept all kinds of characters in these fields, but if the input entered is not a valid number the browser will complain (red border) and it will moreover send an empty string to the server instead of the data entered.

They might allow it to be typed but do they actually save it?

I have tested this item ✅ successfully on ef9a03b

I have tested this item ✅ successfully on ef9a03b

@brianteeman Allows to type but not saves it. Marked successful as I don't see any other problem. All looks good.

The problem I see with this PR is that the default values defined for each of the ports will now be saved in the configuration.php file whereas these variables stayed empty in the past unless you explicitly entered a value in any of the ports fields (exception was and is the SMTP port field where the default was already set to "25".

IMO it would be better if the settings continue to stay empty if the fields are not marked as required and the administrator didn't enter any value (or removed the value previously entered).

A hint instead of a default will fix that

Hint already there, displays on hover of label. So just need to remove default value.

I have some issues with this PR:

Initially the field "$ftp_port" in configuration.php was empty.

After opening the global configuration, clearing the default value "21" from the "FTP Port" field and saving the global configuration (no error messages displayed) the "$ftp_port" field in configuration.php was no longer empty, instead it contained the value "21". Since the "FTP Port" field is not marked as required there should be no value being saved in configuration.php.

I then removed the default attribute from application.xml (as suggested by @hardiktailored), cleared the "$ftp_port" field in configuration.php, logged out and again in and opened the global configuration. Now the value shown in the "FTP Port" field was "1" - I think that in case of no default value the minimum value is used instead.

I again cleared the "FTP Port" field and tried to save the global configuration, but now I got an error message saying that the "FTP Port" field is invalid. But this field is not required, so leaving it empty should be perfectly OK. The value in this field was "1" afterwards, probably again the minimum value.

Next I wrote the value "0.11e+2" into the "FTP Port" field. The browser signaled that this value is OK since the field is a numeric field and this value is a valid numeric value. When I submitted the form this value was sent to the server (I checked it by means of the network analyzer built in into Firefox), and again I got an error mentioning "Invalid FTP Port". The interesting fact is that afterwards the value shown in this field was "11".

Next I wrote the value "1.1e+1" into the "FTP Port" field. Again the browser signaled that this value is OK and it transferred this value to the server. Now there was no error message, instead the value "1" was saved for "$ftp_port" in configuration.php. But when I reopened the global configuration the value shown there was "11" (I did this without logout and new login).

IMO the following should be implemented with respect to numeric fields in forms:

• if the field is not required no default value should be set in the form definition
• if the field is not required and no previous value exists the field should be empty when opening the form
• when saving an empty field that is not required this should be accepted and any previous value should be replaced with an empty value
• there seems to be some discrepancy between the validator "number" and the filter "integer", so maybe a validator "integer" is needed

Regarding the input value "1.1e+1" for the "FTP Port" field I created the PR #11491 some minutes ago which causes the same values being shown in the global configuration form as are saved in configuration.php.

Regarding empty numeric values e.g. for the "FTP Port" field I created the PR #11492 some minutes ago.

I have tested this item ✅ successfully on ef9a03b

Tested this in Firefox with success.
value 'abcd' entered before applying the text was updated to value 1 when the patch was applied.

I see a difference in the behaviour of the fields ftp_port and smtpport, due to the fact that smtpport is required and ftp_Port not.

Emptying both fields show the default value 'grayed' but it is not possible to submit the form. The field smtpport has to be filled with a number, eventually the hint value.

@Luchen6
Regarding the difference you encountered in the behaviour of the fields ftp_port and smtpport: if you fill in e.g. the default value "25" in the smtpport field and leave the ftp_port field empty the latter will nevertheless be filled with the default value "21" (please check in the configuration.php file). This is a change in behaviour compared to the previous implementation, and if you want to have the ftp_port field empty in the configuration file if you didn't enter a number in that field then please check PR #11492.

When testing both this PR (#11322) and then the PR #11492 must be applied, moreover it's necessary to modify the file administrator/components/com_config/model/form/application.xml (remove the line default=21 from the ftp_port field definition). For more details please see the testing instructions for #11492.

In general it should be decided I think whether numeric fields that are not required should have a default value (they should however have a hint with the usual default value if that value is unambiguous). In cases where more than one such value is possible or well-known (as is the case with the proxy_port field) the information about typical values should be given in the field description and neither a default value nor a hint value should be set.

And even for the smtpport field I suggest that it should be discussed whether this field is really required. The default value for the SMTP port depends upon the SMTP security method chosen (the default ports are given in the field description). So maybe even this field should stay empty if the default SMTP port shall be used, and the method useSmtp (in libraries/joomla/mail/mail.php) should evaluate the port to be used depending upon the SMTP security method given if no port ist given.

@jo-sf

I was testing this PR without knowing the presence of PR 11322.
Tested PR 11322 according to the instructions with success.

I am not sure enough to decide whether these fields should have a default value or not. I have not the technical knowledge to decide on this.

@Luchen6
I think neither of us can decide whether these fields should have a default value.

The ftpport field doesn't need a default value since there was none in the past (means: prior to the change done by this PR). The code using this value seems to have a reasonable default value.

Unfortunately this is currently not the case for the smtpport field. As I explaned above the useSmtp method doesn't use a default value according to the SMTP security setting, and so it's currently necessary to configure the SMTP port in the global configuration according to the SMTP security settings chosen. If this method is changed in a way that it chooses the right SMTP port according to the SMTP security setting the smtpport field may stay empty as long as the default port shall be used.

closing as no time to check this. if anyone wants to continue fell free.

I copied the changes to the global configuration form to PR #11492, see there for more details.