[TYPO3-commerce] RFC: #6634: Refactor tx_commerce_products get_attribute_matrix() and get_product_attribute_matrix()

[Morten Olesen]

Hi Christan & Dimitri,


I finally had some time to test the changes, how ever since issue 1345 
remains unsolved it's kinda limited what I can test.

This test is done against svn 19/03-2010 ( patch 12.2 iirc );

your patch introduces
"PHP Warning:  Call-time pass-by-reference has been deprecated" x 3  : 
line 216, 299, 344 in pibase


Other than that it seems to crash randomly when selecting attribute 
combinations - cause is;

(Note; this also happens in unpatched 12.2 - how ever since the patch 
modifies function that gives out the attributes used for creating the 
dropdowns I thougth I migth be good to bring it up aswell.)

Some further investigation reveals that the crashed is not random at 
all, but due to the drop downs getting too many options.

Ie. a product with theese two attributes ( values in parantesis )

Colour ( Blue, Yellow, Beige )
Size ( Large, Small, XLarge )


All those choices are shown in the selects, how ever only the combinations;

     * Yellow - Large
     * Yellow- Small
     * Beige - Large
     * Beige - Small

exists.


(This also happens in 12.2 )

Altering the article order so that they are now

Beige	Small
Yellow	Large
Yellow	Small
Beige	Large


"Yellow, Large" is still chosen by default, where as "Beige, Small" 
would be the logical choice ... (yes we do infact have customers who 
thinks this is important ;) )


Test product;
http://testcommerce.idefahost.dk/index.php?id=12&tx_commerce_pi1[showUid]=7&tx_commerce_pi1[catUid]=8



The above shop contains no modifications outside of the patch and the 
only other commerce module installed is 'com_defaultstock'


So in conclusion it seems your changes does work exactly like the 
original (apart from added warnings) - how ever it's less that desired 
that it does so.

/Morten Olesen

Christian Kuhn wrote:
> Hey,
> 
> Dimitri and me spend a lot of (really braindamaging) time refactoring 
> those methods. They are one of the most ugly and parsetime consuming 
> methods within commerce. Please fire up your own kcachegrind in a 
> realistic environment to see how much we gain with this central cleanup 
> in complex list and detail views.
> 
> Did you ever display a list with 20 products, 10 articles and a 
> realistic list of attributes and wondered why your happy little super 
> server took 20 seconds to render this crap? Those methods are one reason!
> 
> We really need some reviews here to ensure backwards compatibility, we 
> are not sure if we covered every edge case. Please try the patches and 
> give us more feedback, otherwise commerce will in future break exactly 
> with _your_ setup.
> 
> Please provide feedback especially for complex setups like multi 
> languages, tons of value lists and so on.
> 
> Thanks for your contribution
> Christian
> 
> Christian Kuhn wrote:
>> Type: Refactoring
>>
>> Branches: trunk
>>
>> Forge issue ID: http://forge.typo3.org/issues/show/6634
>>
>> Problem: get_attribute_matrix() and get_product_attribute_matrix() 
>> have tons of duplicate code, are messy and slow.
>>
>> Solution: Rewrite the methods from scratch.