Opened 10 years ago
Closed 3 years ago
#31616 closed enhancement (wontfix)
Splitting request_filesystem_credentials into separate functions
Reported by: | jipmoors | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Filesystem API | Keywords: | shiny-updates early has-patch needs-refresh needs-testing |
Focuses: | Cc: |
Description
This allows for more efficient and more readable uses.
Needed for Automatic_Upgrader_Skin and Shiny Updates.
Attachments (5)
Change History (27)
This ticket was mentioned in Slack in #core by johnbillion. View the logs.
10 years ago
#5
@
10 years ago
- Summary changed from Splitting request_filesystem_credentials into seperate functions to Splitting request_filesystem_credentials into separate functions
This ticket was mentioned in Slack in #core by jip. View the logs.
10 years ago
#8
@
10 years ago
Patch functional description:
- Added a function that allows for just retrieving filesystem credentials
get_filesystem_credentials
- Added a function just display the HTML form using supplied credentials and type
request_filesystem_credentials_form
- Added a function to verify all required attributes are present in credentials to know if the credentials are 'usable'
usable_filesystem_credentials
- Modified the
request_filesystem_credentials
to use the newly added functions to provide the functionality and output expected
All functions have respect to previous functionality and some "type" checks have been duplicated. But this seemed impossible to avoid. Once the "type" is set it will be passed down, so it won't have to be retrieved again.
This keeps the old (expected) functionality as it was and adds new ways to retrieve data and control output.
#9
@
10 years ago
- Keywords shiny-updates needs-patch added; has-patch removed
- Milestone changed from Awaiting Review to 4.2
Thank Jimpoors. I've take an initial look through your patch and have a couple of notes. There might be more things that I missed, but overall this is a strong initial patch.
Can you also talk through how you would handle the use cases we need this for. For example: What function (or functions) would you anticipate someone calling to find out if we need to ask user for filesystem credentials?
get_filesystem_method -
var_export isn't really needed for context since it's a string or allow_relaxed_file_ownership since it's a bool. I would also move it down so that the initial ! $context check can run. I would put it above the ( if ! $method ) I would also include $method.
The formatting for the docs on filesystem_method filter seems to have gotten screwed up. Let's keep the alignment.
Otherwsise, looks really good. I like that you are saving everything so we
get_filesystem_credentials -
request_filesystem_credentials filter docs formatting is screwed up
While it's unlikely that anyone is causing the filtered value of request_filesystem_credentials to return a value that triggers empty instead of !=== , there are a number of values that would fit this bill such as
- 0 (0 as an integer)
- 0.0 (0 as a float)
- "0" (0 as a string)
- NULL
- FALSE
- array() (an empty array)
- $var; (a variable declared, but without a value)
Therefore, let's keep the strict empty string check.
request_filesystem_credentials -
request_filesystem_credentials filter docs shouldn't be repeated. The second one should reference the first one.
#10
@
10 years ago
Use case
To check what we have and what might be needed:
$type = get_filesystem_method(); $credentials = get_filesystem_credentials( $form_post, $type ); // check if the credentials are sufficient for specified type: if ( ! usable_filesystem_credentials( $credentials, $type ) ) { // more input is needed request_filesystem_credentials_form( $form_post, $credentials, $type, $error ); } else { // use $credentials in any way you like; spit it out to javascript or modify files }
$form_post is needed to keep legacy code running, the filter that allows for credential-injection by plugins might need this value for checking |
- Type: 'direct'
$credentials
: true
usable_filesystem_credentials( true, 'direct' )
: true
- Type: 'ssh':
$credentials
: holds private/public key if set via DEFINE or $_POST
usable_filesystem_credentials( $credentials, 'ssh' )
: true if both private & public keys are present
- Type: other:
$credentials
: holds username, hostname, port
usable_filesystem_credentials( $credentials, 'ftp' )
: false without password
The original code had these types for checking valid credentials, I could not think of any methods that would need something other than username, password and hostname but if these exist they are easily added inside the usable_filesystem_credentials
function.
@
10 years ago
Added unittests for usable_filesystem_credentials and get_filesystem_credentials. Fixed array index checks because of filtered credential overrides.
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#12
@
10 years ago
One note on the usable_filesystem_credentials
that is a bit counter intuitive.
The $type
parameter is only used to check for 'direct' method when not supplied.
Providing 'direct' with a $credentials
array with 'connection_type' = 'direct'
would help a lot in working properly with this function.
If it's supplied it is not used, because the $credentials['connection_type']
is being checked. This is what will be used on the form and is being used to connect eventually.
Therefore my previous use case is not reflecting how it really works.
Backwards compatibility dictates that the $type
parameter should be there, a clearer documentation is needed to avoid unexpected results.
Will provide addition documentation in patch.
Considerations
Currently testing available methods to address the filesystem and checking for required parameters for a certain connection type are intermixed inside the functions. Preferably these would be split into separate functionality. Though this would require a lot of rewriting.
(edit: added considerations)
This ticket was mentioned in Slack in #core by jip. View the logs.
10 years ago
@
10 years ago
get_filesystem_credentials returns array for type 'direct' with proper connection_type
#14
@
10 years ago
Update use case (since patch 3)
Patch 3 modifies the get_filesystem_credentials to have 'direct' connection return a similar $credentials
array with 'connection_type' set to 'direct'. This allows for more unified code.
if ( 'direct' == get_filesystem_method() ) { // no need for credential checks; though they will pass } // form_post might be needed for credential filter; can't be sure plugins don't use it though seems highly unlikely. $credentials = get_filesystem_credentials( $form_post ); // check if the credentials are sufficient for specified type: if ( ! usable_filesystem_credentials( $credentials ) ) { // more input is needed request_filesystem_credentials_form( $form_post, $credentials, '', $error ); } else { // use $credentials in any way you like; spit it out to javascript or modify files // explicit check for $credentials['connection_type'] == 'direct' is possible here, but not required }
The only parameter accepted (and required) for usable_filesystem_credentials
is now an array. Required parameters are checked on 'connection_type' with a fall through to 'hostname', 'username', 'password' for anything except 'direct' and 'ssh'.
#15
@
10 years ago
request_filesystem_credentials filter
In what way is this filter used and how do we maintain expected functionality?
Currently usable_filesystem_credentials
expects and array with a certain formatting depending on the connection_type
.
Would it be necessary to add a filter to that function to override the check to comply with 'custom' implementation (however that may look).
Changes
With the request_filesystem_credentials
functionality, if the filter returns something the entire function is skipped.
When the new functionality is implemented the old function keeps working like expected.
But new functionality using supplied use case will not exit when data from the filter is retrieved but tries to validate the formatting of that data. There is no way to know where the credentials are coming from.
Will this pose problems?
What we don't want
Adding additional data to the output of the filter shouldn't be an option. Thus 'tagging' that it came from a filter is out.
The filter request takes a lot of parameters, so re-using that filter to check if the credentials match a filter request is out aswel.
Adding a filter inside usable_filesystem_credentials
to allow for overriding internal checks looks to me like an unwanted and complicated way of attacking this possible problem. Mostly because the filter has been around since 2.5.0, so adding a new filter that is needed to achieve 100% coverage is not an option.
This ticket was mentioned in Slack in #core by jip. View the logs.
10 years ago
#17
@
10 years ago
I've been blind apparently.
/** * Plugins may override this form by returning true|false via the * {@see 'request_filesystem_credentials'} filter. */
So there are no credentials being returned by this filter.
Though the implementation of the return value does not reflect this comment. If anything is returned the form is not displayed.
@
10 years ago
Reformatted docs. Converted function parameters to optional args array. Fixed filter location. Organised tests.
Splitting the functionality into separate functions; adding verification function for credential parameters.