Skip to content

Added support for using PHP array callbacks. Refactored get_mapped_mvc_call#32

Open
niktest wants to merge 4 commits into10quality:v3.0from
niktest:array-callbacks
Open

Added support for using PHP array callbacks. Refactored get_mapped_mvc_call#32
niktest wants to merge 4 commits into10quality:v3.0from
niktest:array-callbacks

Conversation

@niktest
Copy link

@niktest niktest commented Jun 6, 2023

Adding support for using PHP array Callbacks / Callables to add_action, add_filter, add_shortcode, remove_action, remove_filter. Simplified the preg_match call with a stripos to optimize performance.

This will allow using the below approach, which can offer better modules dependencies and their methods tracking in the popular IDE's

        $this->add_action('wp_enqueue_scripts', [AppController::class, 'wp_enqueue_scripts']);
        $this->add_shortcode('components_example', [AppController::class, 'components_example']);

…c_call

Adding support for using PHP array Callbacks / Callables to add_action, add_filter, add_shortcode, remove_action, remove_filter.
Simplified the preg_match call with a stripos to optimize performance.
$type_prefix = stripos( $call, 'view@' ) !== false ? '_v_' : '_c_';
$return_prefix = $return ? 'return_' : 'void_';

return $type_prefix . $return_prefix . $call;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To optimize performance and improve memory allocation, we want to avoid instantiating variables unless is highly necessary. Please do the concatenation directly, the same as the original code that was refactored, and remove all the variables that are only used once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rolled back.

$call = str_replace( $class_prefix, '', implode( '@', $call ) );
}

$type_prefix = stripos( $call, 'view@' ) !== false ? '_v_' : '_c_';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow the regex pattern added in the original, this is not covering the original functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no worries. I had restored original code and styling

. ( $return ? 'return_' : 'void_' )
. $call;
if ( is_array( $call ) ) {
$class_prefix = $this->config->get( 'namespace' ) . '\\Controllers\\';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you that /Controllers/ will always be the route?, take into consideration that the path can be changed at config/app.php, so this PR is not covering all case scenarios.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I've rewritten this controversial solution.

return ( preg_match( '/[vV]iew\@/', $call ) ? '_v_' : '_c_' )
. ( $return ? 'return_' : 'void_' )
. $call;
if ( is_array( $call ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any new feature like this requires unit testing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extended the tests along with the new functionality

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.

2 participants