WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 months ago

#31616 new enhancement

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)

31616.diff (25.0 KB) - added by jipmoors 4 years ago.
31616.1.diff (16.3 KB) - added by jipmoors 4 years ago.
Fixed formatting and docs. Applied the functionality improvements.
31616.2.diff (20.7 KB) - added by jipmoors 4 years ago.
Added unittests for usable_filesystem_credentials and get_filesystem_credentials. Fixed array index checks because of filtered credential overrides.
31616.3.diff (20.5 KB) - added by jipmoors 4 years ago.
get_filesystem_credentials returns array for type 'direct' with proper connection_type
31616.4.diff (29.2 KB) - added by jipmoors 4 years ago.
Reformatted docs. Converted function parameters to optional args array. Fixed filter location. Organised tests.

Download all attachments as: .zip

Change History (26)

@jipmoors
4 years ago

#1 @jipmoors
4 years ago

  • Keywords has-patch added

Splitting the functionality into separate functions; adding verification function for credential parameters.

#2 @jipmoors
4 years ago

  • Component changed from Plugins to Filesystem API

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


4 years ago

#5 @SergeyBiryukov
4 years ago

  • Summary changed from Splitting request_filesystem_credentials into seperate functions to Splitting request_filesystem_credentials into separate functions

#6 @jipmoors
4 years ago

Relevant #29820

This ticket was mentioned in Slack in #core by jip. View the logs.


4 years ago

#8 @jipmoors
4 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 @jorbin
4 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 @jipmoors
4 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.

Last edited 4 years ago by jipmoors (previous) (diff)

@jipmoors
4 years ago

Fixed formatting and docs. Applied the functionality improvements.

@jipmoors
4 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.


4 years ago

#12 @jipmoors
4 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)

Last edited 4 years ago by jipmoors (previous) (diff)

This ticket was mentioned in Slack in #core by jip. View the logs.


4 years ago

@jipmoors
4 years ago

get_filesystem_credentials returns array for type 'direct' with proper connection_type

#14 @jipmoors
4 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'.

Last edited 4 years ago by jipmoors (previous) (diff)

#15 @jipmoors
4 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.

Last edited 4 years ago by jipmoors (previous) (diff)

This ticket was mentioned in Slack in #core by jip. View the logs.


4 years ago

#17 @jipmoors
4 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.

@jipmoors
4 years ago

Reformatted docs. Converted function parameters to optional args array. Fixed filter location. Organised tests.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 years ago

#19 @jorbin
4 years ago

  • Keywords early added
  • Milestone changed from 4.2 to Future Release

As nice as this would be, I think it is best to get a refactor like this included early in a future release rather than late in this one.

#20 @jipmoors
4 years ago

  • Keywords has-patch added; needs-patch removed

#21 @jipmoors
4 years ago

  • Keywords needs-refresh needs-testing added
Note: See TracTickets for help on using tickets.