Customer class updates for v2.0.0

[lat9]

Customer.php Class

The current (zc158a) version of includes/classes/Customer.php is a good start at encapsulating various customer-related functions for common use, but there are a couple of issues with its current implementation:

1. Performance. The class' instantiation gathers all information associated with the customer, e.g. their full address-book. While those queries are cached via the $db methods , the additional processing is unnecessary for much of its use. For example, the zen_get_customers_address_primary instantiates the class solely to retrieve the id associated with the customer's primary address-book entry.
2. Data Staleness. Since the class has no control of the customer-related database changes outside of its scope, subsequent method-accesses post-instantiation could have "stale" information in the class' data property. For example, if the class is instantiated by a script and that script updates an element of the customer's database-related information, further use of class methods by that script could return "stale" data.
3. Guest-customer Interactions. While the class recognizes that a "guest" customer is logged in, its data-returning methods presume that database information associated with that guest-customer's customers_id contains valid information — which is not necessarily valid.

Note: These changes would be used as the basis for updating the class in support of the Wholesale Pricing addition.

Proposed Changes

Performance

1. Update the class' public __construct method to take an optional (bool)$load_all_data input (which defaults to true for downward-compatibility). The value is stored as a class property for subsequent use by the load method.
2. Update the class' protected load method to inspect the class' (bool)$loadAllData property to see whether all information, e.g. the full address-book, should be gathered or whether only the base customers and customers_info tables' data should be gathered.
3. Add a setLoadAllData method so that a class user can modify the processing flag post-instantiation.

Data Staleness

1. Use $db->ExecuteNoCache instead of $db->Execute for all queries. While this will affect performance, it will ensure data integrity.

Guest-customer Interactions

1. If a guest-customer is currently active, treat any data-related requests as if the customer does not exist.

Current In-Core Users of the Class

admin/customers.php

Uses the Customer class for form-related data-gathering and some of its actions. Performs direct database updates outside of the class when an admin has edited a customer's information.

includes/functions/functions_customer.php

The majority of the functions instantiate the class simply to retrieve a specific customer value.

includes/modules/checkout_address_book.php

Uses the class to gather the various address-book records associated with the currently logged-in customer.

includes/modules/create_account.php

Uses the class to create a customer's account and, if successful, login that customer.

includes/modules/pages/account/header_php.php

Uses the class to gather the currently-logged-in customer's most recent orders.

includes/modules/pages/account_history/header_php.php

Uses the class to gather the currently-logged-in customer's full order history.

includes/modules/pages/account_history_info/header_php.php

Uses the class to ensure that the currently-loaded order is one placed by the currently-logged-in customer.

includes/modules/pages/address_book/header_php.php

Uses the class to gather all the currently-logged-in customer's formatted address-book entries.

includes/modules/pages/login/header_php.php

Uses the class to ensure that the page's form's email address is associated with a customer account and, if so, uses the login method to log the customer into the site's storefront.

[drbyte]

Comments/questions, in no particular order:

• admin/customers.php -- Do we know how many plugins directly modify the update-queries contained therein? (or at least which ones customize that file.) There would be great gains by moving most/all of those db queries into the class, instead of leaving them in the admin/customers.php file.

• note that the create-account and login processing logic must know the customer's default address id in order to set the SESSION variables for those values that are used by other parts of the code (shipping estimator, checkout, etc). There would be value in changing those other code areas to do bespoke lookups if the session value is not defined ... but of course plugins that expect it to be present will encounter PHP errors.

• guest -- are there sections of the application that expect a customer object to simulate as guest?
  ... and, somewhat related, how will paypalwpp handle "no customer" if treated as a guest that doesn't exist?

[lat9]

• Agreed on moving the various customer-update functions into the class. I don't personally have any sites that use plugins that make use of the class.
• The login method sets the various $_SESSION variables upon successful completion.
• I'm not sure what "the application" refers to. For paypalwpp, review of that and the ipn_main_handler.php would be warranted to get a feeling for their usage model.

[drbyte]

I'm not sure what "the application" refers to.

er ... "the application" = "Zen Cart"

[drbyte]

After code inspection, it turns out that the admin/customers.php file does only one set of updates to customer data, and that's when the Admin actually does an Edit operation.
All the validation and sanitization are done in that file, and a direct zen_db_perform() call is made to update the database. Then the page is reloaded, which also reloads customer data from the database.

So, there's nothing really to remove from admin/customers.php.

I suppose we could move all validation logic for all customer fields into the class. That would allow both admin and catalog to share the validation logic instead of repeating various parts of it in both places. Centralizing that into a class would allow an API to interact with customer-create/edit requests.

[drbyte]

2. Update the class' protected load method to inspect the class' (bool)$loadAllData property to see whether all information, e.g. the full address-book, should be gathered or whether only the base customers and customers_info tables' data should be gathered.

What about having the getData(?$element) method do more logic where if a property it's asked to lookup hasn't been loaded yet, it goes and loads it on-demand ... and if no param is passed to it (ie: it's expected to return "all"), and for the many places that call getData() without a lookup element, perhaps some of those could be more granular/specific.

Perhaps the getData($element) call could accept an array of element keys instead of just a single string?

As far as the "granular" suggestion, the load() method is very large and could afford to be split up into several smaller methods of a patterned naming convention that the getData($element) function could use directly to load something or perhaps pass along to the load method to broker it through any dependencies it may have.

[lat9]

I like the idea of having a method, perhaps getDataArray, through which a caller could request a collection of customer-related values.

[drbyte]

To be clear, are you saying you dislike letting the getData method accept either a string or an array?
IMO having two functions that do the same thing but allow a different type of input parameter seems duplicitous.

[lat9]

Just because there are two public methods available that have strongly-typed inputs doesn't mean that they couldn't funnel the processing to a common protected method!

[drbyte]

Should the $customer object be made more widely accessible so that the functions_customers calls to the class don't have to re-instantiate afresh needlessly?
(Or, perhaps those functions are only for backwards-compatibility, and mostly not-used in core code anymore, so the re-instantiation issue is a moot point.)

[lat9]

I'll go with the "functions are only for backwards-compatibility".

[drbyte]

While some of those functions are indeed used in core code, cursory review suggests that all could be replaced with direct calls to the Customer class's public methods.

[drbyte]

I've reviewed functions_customers.php. I reworked a couple to use the class. A couple remaining ones do some direct queries but I'm not bothered about those for now.

[lat9]

I'm also not liking the class' presumption about $_POST variables:

zencart/includes/classes/Customer.php

Lines 732 to 734 in 83fa36e

	if (ACCOUNT_DOB == 'true') {
	$sql_data_array[] = ['fieldName'=>'customers_dob', 'value'=>empty($_POST['dob']) || $data['dob'] == '0001-01-01 00:00:00' ? ('0001-01-01 00:00:00') : zen_date_raw($_POST['dob']), 'type'=>'date'];
	}

Shouldn't that 'dob' value be part of the submitted $data array?

[drbyte]

Yes, dob is already part of the $data array, and should be used ahead of POST data

[drbyte]

FYI, I pushed an update to remove POST use from the class. It was only with the dob data, which was already in the array anyway.

[zcwilt]

Just to throw a curve ball in here
But we have a Laravel model for the customer class
Where possible would like to see us removing old queries to use eloquent

[drbyte]

If that's fully accessible from both Admin and Catalog, then it would make sense to use that.

[drbyte]

Proposed Changes
Data Staleness
Use $db->ExecuteNoCache instead of $db->Execute for all queries. While this will affect performance, it will ensure data integrity.

Does this entirely solve the "staleness" problem though?

I would think that core code would need to be changed to actually ask for less-stale details, else the NoCache approach is pointless.

[lat9]

Certainly, there are possibilities that customer changes to their addresses, for instance, could collide with an admin-related update, but using the ExecuteNoCache method to retrieve the requested data would at least close that gap.