godaddy-wordpress / wc-plugin-framework

The official SkyVerge WooCommerce plugin framework
Other
138 stars 42 forks source link

Allow SV_WC_Plugin::load_class() to pass one or more arguments to __construct() #145

Closed unfulvio closed 5 years ago

unfulvio commented 8 years ago

should we add the ability to this method to pass an undefined/variable number of params after the first two (file name and class name) to pass an equal amount of params to the constructor of that class?

https://github.com/skyverge/wc-plugin-framework/blob/master/woocommerce/class-sv-wc-plugin.php#L526

so it can do new Class_Name( $arg1, $arg2 ... );

maxrice commented 8 years ago

yeah I waffled on adding this in the method originally, but I decided to just keep it simple and focused on one-lining our previous pattern:

require_once( ...);
$this->some_instance = new $some_class();

If we have a lot of usages of that same pattern where constructor arguments are needed, I'm open to adding this sort of thing. Can you work up a count?

unfulvio commented 8 years ago

ok will do, it's not urgent -- just something I thought while working at Memberships at a class that might require this (but later I realized it can be reworked differently, it's still wip)

likely we are using it to handle "loaders" those shouldn't contain variables, but yeah can have this issue open as a placeholder for keeping check of actual usage

unfulvio commented 8 years ago

since I opened this issue I did not need anymore to pass an argument in constructor, I've found just now a use case in PDF Product Vouchers and adding a note here:

https://github.com/skyverge/wc-plugins/blob/master/woocommerce-pdf-product-vouchers/woocommerce-pdf-product-vouchers.php#L266-L276

classes WC_PDF_Product_Vouchers_Product and WC_PDF_Product_Vouchers_Voucher

although at first glance doesn't look like a nice pattern, because they pass the whole instance of the main class to those objects while instantiating them... could it be a remnant of a pre-singleton pattern?

edit: there's also WC_PDF_Product_Vouchers_Admin passing $this from main class

unfulvio commented 5 years ago

as we're moving to modern PHP codebase and namespaces/autoloaders this issue from 2016 is likely obsolete and I'm going to close it - feel free to reopen if still applicable