Surely we only need to display one value.

On 23 Jul 2016 7:24 a.m., "Izhar Aazmi" notifications@github.com wrote:

Summary of Changes

Displaying max upload limit calculated from PHP configuration values in
upload package installer page.

To my knowledge, uploading a file larger than the post_max_size limit
will still raise PHP warning etc. which is raised even before the
Joomla/PHP script is executed. So we do not have any control over it.
Therefore, I think it should be better to let the user know the limits
beforehand.

Exceeding only the upload_max_size only (when it is set lower than the
post_max_size) gives us the chance however to detect a failed upload from
our code.

However, since this displays PHP settings to the end user I'd not mind
getting this proposal rejected on security grounds. Though, I think only
the users having access to installer interface are concerned here.
Testing Instructions

Apply patch #11026 #11026
(dependency)
Apply this patch afterwards.

Check the installer page - extension package upload tab. It should show
the correct limits as configured in your PHP INI. Change the INI settings
and make sure the values are reflected correctly again.

[image: image]

https://cloud.githubusercontent.com/assets/6706189/17075964/01b90314-50c3-11e6-9f04-7f0af392bf22.png

You can view, comment on, or merge this pull request online at:

#11255
Commit Summary

• Show configured upload limit when uploading installer package. Otherwise when uploading a larger file than the configured limit, the page reloads without any information about what went wrong.

File Changes

• M administrator/language/en-GB/en-GB.plg_installer_packageinstaller.ini https://github.com/joomla/joomla-cms/pull/11255/files#diff-0 (5)
• M plugins/installer/packageinstaller/tmpl/default.php https://github.com/joomla/joomla-cms/pull/11255/files#diff-1 (19)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/11255.patch
• https://github.com/joomla/joomla-cms/pull/11255.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11255, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8eznC8mVEUH2w4rjD85U77_kCEK6ks5qYaVxgaJpZM4JTSb4
.

I thought about it. But I put all the three factors that affect it to let user know the limiting factor as well. I also thought of showing only the limiting factor, but that was not better than all/one.
For me its ok to reduce the amount of information displayed.

What do you suggest among these:

1. Final upload size only
2. Final upload size + the limiting factor
3. Final upload size + post max size + max upload size
4. Final upload size + all 3 PHP settings (rejected already)

Plus if we are going to do it here we should do it in the media manager and the template manager option screens where you can configure the maximum allowed upload size.

And displaying it in bytes is not needed at all. it just complicates things with unnecessary information

Yes, media manager is on my todo. Now included the template manager options too.

You only need to display the one that is in effect

Done removing extra information.

I have tested this item ✅ successfully on 1ee34bd

That looks better - will you be adding this change everywhere in this PR or
in other PRs?

On 23 July 2016 at 13:22, bertmert notifications@github.com wrote:

I have tested this item ✅ successfully on 1ee34bd

1ee34bd

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/11255
https://issues.joomla.org/tracker/joomla-cms/11255.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11255 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8ZEAhk3Dze8-4LwdMFMLvGH7kqeWks5qYflsgaJpZM4JTSb4
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@izharaazmi I think @brianteeman means to move the code to the field directly (DRY) https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/form/field/file.php

@dgt41 Thanks for the tip.

@brianteeman I can add all of them here itself (or a separate PR if you say).

Since the calculation is to be used at several (5 that I know for now) places therefore I think we should add a JHtml method for it something like JHtml::_('max_upload_size'); but I am not sure where! Can you advise?

@izharaazmi I would suggest to move the logic here:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/file.php
and then in plugins/installer/packageinstaller/tmpl/default.php use jlayout to render the field. This way the change will be site-wide (wherever the field is used)

The plugins/installer/packageinstaller/tmpl/default.php and other 4 places that I can see for now are using a file input as plain as <input required type="file" id="upload-file" name="Filedata[]" multiple /> and even <input type="file" name="files" required /> converting them to use JFormFieldFile would be too much.
About rendering all of them directly using the JLayout and not via the JFormFieldFile would require the function logic within the layout file. I don't think layouts are the place for function logic.

Added benefit of adding this to library would be to provide 3PD to reuse it anywhere.

I have tested this item ✅ successfully on 1ee34bd

The maximum upload size is displayed correctly, very nice! However, the first tab 'Upload Package File' was initially not selected by default anymore. This used to be so, though. I'm not sure if this behaviour is due to this patch.

I have tested this item ✅ successfully on 1ee34bd

@izharaazmi patch works properly, but please apply suggestions from @dgt41

@jeckodevelopment The suggestions from @dgt41 is in my todo already that would add this functionality to the JFormFieldFile, however that is for another PR.

For the current cases it is not sufficient as not all instances of file input are rendered using JFormFieldFile. For details see #11255 (comment)

I am waiting for a good idea / suggestion about where to move the calculation part so that it is not repeated everywhere we implement this feature, therefore holding the fixes on other place within CMS.

@brianteeman Should I create it as a function uploadMax in JHtmlPhpSettings? Or any better suggestion?

I moved the calculation part to JUtility::getMaxUploadSize() and applied this change to all the places where file input is displayed in Joomla:

• Extensions > Install > package Installer
• Joomla update > Upload & Update
• JFormFieldFile > input
• Template manager > template > customise > new file (isis + hathor)
• Content > Media > upload (com_media)
• New Article > Wysiwyg Editor > upload images (com_media)

If I'm still missing some places please do let me know.

I have tested this item ✅ successfully on

? Works great! On more tester please

I have tested this item ✅ successfully on 5e9ab10

RTC

Shit, merged it in staging but should be merged into 3.7.x, going to revert this.

@izharaazmi could you make a PR against the 3.7.x branch, thanks

@rdeutz Please check #12396