-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[4.0] [cosmetics] Remove variable type mixing in display controller / view #30043
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
Conversation
1. should we define the property in the view in which we want to fill it later 2. we should use the set method from the base class 3. the marking as a reference is no longer necessary
administrator/components/com_joomlaupdate/src/Controller/DisplayController.php
Outdated
Show resolved
Hide resolved
administrator/components/com_joomlaupdate/src/Controller/DisplayController.php
Show resolved
Hide resolved
administrator/components/com_joomlaupdate/src/Controller/DisplayController.php
Show resolved
Hide resolved
|
I have tested this item ✅ successfully on e57dc08 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30043. |
|
I have tested this item ✅ successfully on e57dc08 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30043. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30043. |
|
I want a practical test of this too please as it covers Joomla Update. Code review is good enough for one test but not both |
@degobbis Please provide testing instructions for a practical test. Thanks in advance. |
|
@richard67 @wilsonge |
|
While playing through the test instruction I noticed that a test is very complex and not easy to do. Since the FTP credentials were implemented as a workaround for the wwwrun problem on web servers, such a server is necessary for a reliable test. Already the installation of Joomla on such a server is the first hurdle. At least two installations are necessary, because resetting the installation after the update is not so easy and could falsify the result because of the file permissions. Do we have professionals who could test this? |
|
There is another PR dealing with Joompaupdate Upload & Update and FTP: #28029 . Maybe the author, @twister65 can help with testing here? Or maybe @Quy can do? |
|
OK Doesn't look like we'll get testers :/ Thanks |
Summary of Changes
A better way to set the FTP credentials for the view1. we should define the property in the view in which we want to fill it later2. we should use the set method from the base class to set a value3. the marking as a reference is no longer necessaryAfter further research it seems that the variable $this->ftp holding the return value of the setCredentialsFromRequest() method is not used and / or directly overwritten in the associated view with the FTP credentials from the model.
Testing Instructions
Code review