Skip to content
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

New SettingsInterface #2505

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft

New SettingsInterface #2505

wants to merge 1 commit into from

Conversation

stracker-phil
Copy link
Collaborator

@stracker-phil stracker-phil commented Aug 12, 2024

Description

Simplifies settings-access for developers, and helps to keep code concise and easy to maintain.

Reason

The current Settings class, which implements the PSR ContainerInterface, led to several challenges in our codebase.

  1. Developers often need to use complex and repetitive patterns to safely retrieve settings
  2. Static analysis tools frequently raise warnings about potential unhandled exceptions. Those exceptions need to handled instantly or propagated via @throws annotations.

New SettingsGetterInterface

The Settings class implements the new SettingsGetterInterface which provides the single method get_value()

  1. Improves developer experience with a more intuitive and WordPress-ish API, without the possibility of throwing an
    exception
  2. Better support static analysis tools - no throws, ternary operators
  3. Maintains backward compatibility with existing code for gradual adoption

Before-After Comparison

Current

/** @throws NotFoundException */
function ( ContainerInterface $container ): bool {
    $settings = $container->get( 'wcgateway.settings' );
    assert( $settings instanceof ContainerInterface );

    return $settings->has( 'allow_card_button_gateway' ) ?
        (bool) $settings->get( 'allow_card_button_gateway' ) :
        $container->get( 'wcgateway.settings.allow_card_button_gateway.default' );
}

...

$type = $this->settings->has( 'applepay_button_type' ) ?
    $this->settings->get( 'applepay_button_type' ) : 
    '';
$checkout_data_mode = $this->settings->has( 'applepay_checkout_data_mode' ) ? 
    $this->settings->get( 'applepay_checkout_data_mode' ) :
    PropertiesDictionary::BILLING_DATA_MODE_DEFAULT;

Downsides

  • ContainerInterface::get() can throw an exception that needs to be handled
  • Prone to errors due to repeated settings-key string
  • Not the best pattern for accessing setting values

Proposed

function ( ContainerInterface $container ): bool {
    $settings = $container->get( 'wcgateway.settings' );
    assert( $settings instanceof SettingsGetterInterface );

    return $settings->get_value(
        'allow_card_button_gateway',
        (bool) $container->get( 'wcgateway.settings.allow_card_button_gateway.default' )
     );
}

...

$type = $this->settings->get_value( 'applepay_button_type', '' );
$checkout_data_mode = $this->settings->get_value(
    'applepay_checkout_data_mode',
    PropertiesDictionary::BILLING_DATA_MODE_DEFAULT
);

Benefits

  • No Exception handling, or annotation
  • Concise and more intuitive to maintain
  • Same pattern that's used by WordPress (get_option(), get_post_meta(), etc.) of always returning a value and never throwing anything

Change ideas

  • Add logging to the get_value() method to log access to undefined setting keys.

@stracker-phil stracker-phil changed the title ✨ Introduce SettingsGetterInterface New SettingsInterface Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant