New event so more validation classes can be added on the fly

[woutersamaey]

Description (*)

I ran into a situation where I needed more complex validation rules like min and max length for strings and min and max values for integers. Without this change, I cannot make it.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[woutersamaey]

WHat are your thoughts @kiatng @colinmollenhour @sreichel ?

[kiatng]

Is it for product or customer? Can you provide an example?

[woutersamaey]

This event can be used for any EAV attribute, but I'm using it for stricter product data entry. My customer has a huge database and wants extreme strictness on all data entry.
They have fields that should be exactly 13 digits and things like that. Since they have so many fields I'm making everything configurable by adding extra fields to the eav_attribute table for max_length, min_length, etc, so not going the way of 1 custom validation class. That's too limited and I'd need to create too many.

One generic sample I can give it this, but I'd doing way more...

/* @var $attribute Mage_Eav_Model_Entity_Attribute */
$attribute = $observer->getAttribute();

/* @var $classes ArrayObject */
$classes = $observer->getClasses();

$backend = $attribute->getBackendType();

if ($backend === 'varchar') {
     $classes[] = 'validate-length';
     $classes[] = 'maximum-length-255';
}

[kiatng]

I see, the class names are intended for validation.js. I implemented something similar, all I did was simply update the classes directly into the table eav_attribute, with a space between the class names:

For me, it was for customer attributes, updated with a simple script:

    'tdn' => [
        'frontend_label'    => ['Passport'],
        'used_in_forms'     => ['adminhtml_customer', 'customer_account_edit', 'ap_employee', 'ap_dependent'],
        'apply_to'          => [$groupIds['Member'], $groupIds['Dependent']],
        'frontend_class'    => 'validate-length maximum-length-9 minimum-length-4',
        'sort_order'        => 40
    ],

[woutersamaey]

Putting this in the DB is what I used to do, but it fails when you open an attribute and save it in Magento Admin.
Magento has a frontend_class field in the Edit Attribute form, so whatever value you had, is lost upon save. Magento uses a single select field, so you cannot enter multiple values and the list of options is also too restricted.
The admin cannot make fine-grained validation settings and breaks the custom rules I put in.
I'm working on expanding this form as well, but still need this event.

[Flyingmana]

is it ok if I move the target to the 20.0 Branch?

[woutersamaey]

Yes sure.

About v20, is this stable or... I haven't checked it out yet.

[Flyingmana]

ok, then I will change it later.

v20 is similar stable, mostly because it does not contain much additional changes currently https://github.com/OpenMage/magento-lts/blob/20.0/.github/changelog/version_20.txt
its just better suited for changes, which are technically no bug fixes

[kiatng]

Sorry, my earlier comment is adding new customer attributes, only addressing part of the issue. It has been more than 5 years, and I forgot about the other part, resetting of the frontend_class when saving the attributes. We fixed it by adding additional class names to the dropdown Input Validation for Store Owner:

        /* fix resetting of frontend_class field */        
        $frontendInputElm = $form->getElement('frontend_class');
        $additionalTypes = array(            
            array(
                'value' => 'validate-upper-alphanum',
                'label' => Mage::helper('extendedcustomer')->__('Letters (A-Z) or Numbers (0-9)')
            ),
            array(
                'value' => 'validate-no-future-date',
                'label' => Mage::helper('extendedcustomer')->__('No future date')
            ),
            array(
                'value' => 'validate-length maximum-length-9 minimum-length-4',
                'label' => Mage::helper('extendedcustomer')->__('Validate passport #')
            ),
            // ... more types
        );
         /** @see Varien_Data_Form_Element_Select */
        $frontendInputValues = array_merge($frontendInputElm->getValues(), $additionalTypes);
        $frontendInputElm->setValues($frontendInputValues); 

The above is an override, but perhaps a better solution is adding the $additionalTypes in config.xml, or change the dropdown of Input Validation for Store Owner to text box. This way, we can maintain configurable class names.

BTW, there is also a resetting of source_model when saving attributes, see PR #1293.

[kiatng]

Revisiting our old code again, and I found this:

magento-lts/app/code/core/Mage/Eav/Helper/Data.php

Lines 80 to 110 in 08d41c2

	/**
	* Return merged default and entity type frontend classes value label array
	*
	* @param string $entityTypeCode
	* @return array
	*/
	public function getFrontendClasses($entityTypeCode)
	{
	$_defaultClasses = $this->_getDefaultFrontendClasses();
	if (isset($this->_entityTypeFrontendClasses[$entityTypeCode])) {
	return array_merge(
	$_defaultClasses,
	$this->_entityTypeFrontendClasses[$entityTypeCode]
	);
	}
	$_entityTypeClasses = Mage::app()->getConfig()
	->getNode('global/eav_frontendclasses/' . $entityTypeCode);
	if ($_entityTypeClasses) {
	foreach ($_entityTypeClasses->children() as $item) {
	$this->_entityTypeFrontendClasses[$entityTypeCode][] = array(
	'value' => (string)$item->value,
	'label' => (string)$item->label
	);
	}
	return array_merge(
	$_defaultClasses,
	$this->_entityTypeFrontendClasses[$entityTypeCode]
	);
	}
	return $_defaultClasses;
	}

Lines 95-96:

     $_entityTypeClasses = Mage::app()->getConfig() 
             ->getNode('global/eav_frontendclasses/' . $entityTypeCode); 

That's just what this is:

... a better solution is adding the $additionalTypes in config.xml.

I just saw that, I'll try it to see if it works.

[kiatng]

I am happy to report that the following works for me:

<!-- in your custom module /etc/config.xml -->
    <global>
        <!-- ... -->
        <eav_frontendclasses>
            <customer>
                <passport>
                    <value>validate-length maximum-length-9 minimum-length-4</value>
                    <label>Validate passport #</label>
                </passport>
                <abc>
                    <value>abc</value>
                    <label>Validate ABC</label>
                </abc>
            </customer>
        </eav_frontendclasses>
    </global>

For $result = Mage::helper('eav')->getFrontendClasses('customer'), the output is

 array(9) {
  [0] => array(2) {
    ["value"] => string(0) ""
    ["label"] => string(4) "None"
  }
  [1] => array(2) {
    ["value"] => string(15) "validate-number"
    ["label"] => string(14) "Decimal Number"
  }
  [2] => array(2) {
    ["value"] => string(15) "validate-digits"
    ["label"] => string(14) "Integer Number"
  }
  [3] => array(2) {
    ["value"] => string(14) "validate-email"
    ["label"] => string(5) "Email"
  }
  [4] => array(2) {
    ["value"] => string(12) "validate-url"
    ["label"] => string(3) "URL"
  }
  [5] => array(2) {
    ["value"] => string(14) "validate-alpha"
    ["label"] => string(7) "Letters"
  }
  [6] => array(2) {
    ["value"] => string(17) "validate-alphanum"
    ["label"] => string(35) "Letters (a-z, A-Z) or Numbers (0-9)"
  }
  [7] => array(2) {
    ["value"] => string(49) "validate-length maximum-length-9 minimum-length-4"
    ["label"] => string(19) "Validate passport #"
  }
  [8] => array(2) {
    ["value"] => string(3) "abc"
    ["label"] => string(12) "Validate ABC"
  }
}

@woutersamaey Please try to see if this works for you. If it does, it provides configurable frontend_class in backend and should better address your issue.

[woutersamaey]

Thank you both for the detailed suggestions, you're very kind. But as I said, I'm doing way more complex stuff and I need this event to make it all come together. I need to determine and change validation classes based on custom logic, not just because of what's in the DB.

This screenshot may help give you an idea. Notice the column on the right (and ignore the multiselect rows - this is still a bug and WIP. These values should be NULL)

I hope you can see the benefit to this extra event (not sure why anyone would be against it - but okay).
There simply is no alternative, because this class is extended too many times.

[colinmollenhour]

I'm still not seeing why the "Input Validation for Store Owner" plus config.xml solution that @kiatng pointed out doesn't solve it.. I suppose you want the user to be able to enter any arbitrary length at any time? But surely there are only so many possible lengths you could need to use and these could be added to the config.xml ahead of time. Anyway, no objection to adding another event, that is quite harmless.

[kiatng]

Ahhh... I see that the value of Max Length can be any integer input by a backend user, which cannot be pre-determined, and in which the validation rule is dependent on. It makes sense to me now.

Instead of a targeted event for the function getClass(), I would like to propose something for broader use cases, which can be achieved by dispatching the event here

magento-lts/app/code/core/Mage/Eav/Model/Entity/Attribute/Frontend/Abstract.php

Lines 45 to 55 in 7526749

	/**
	* Set attribute instance
	*
	* @param Mage_Eav_Model_Entity_Attribute_Abstract $attribute
	* @return Mage_Eav_Model_Entity_Attribute_Frontend_Abstract
	*/
	public function setAttribute($attribute)
	{
	$this->_attribute = $attribute;
	return $this;
	}

     public function setAttribute($attribute)
     {
         Mage::dispatchEvent('eav_entity_attribute_frontend_set_attribute', ['attribute' => $attribute]);
         $this->_attribute = $attribute; 
         return $this; 
     } 

This allows other customization of the attribute's frontend properties.

@woutersamaey 's observer can be:

/** @var Mage_Eav_Model_Entity_Attribute $attribute */
$attribute = $observer->getAttribute();
$classes = [$attribute->getFrontendClass()];

$backend = $attribute->getBackendType();

if ($backend === 'varchar') {
     $classes[] = 'validate-length';
     $classes[] = 'maximum-length-255';
}

$attribute->setFrontendClass($classes);

Is that OK? Can you test to see if it works for you?

[fballiano]

I switched the branch to 20.0 as said before in the thread.

@woutersamaey what would you answer to last @kiatng comment? It would be nice (after all this work) to merge this PR :-)

[sreichel]

Why to change array to ArrayObject?

However ... for ArrayObject append() should be used to add elements.

[addison74]

I would put such PRs in the Draft, especially since there is also a conflict to be resolved, and if there is no feedback from the author within a reasonable time, it should be closed by mutual agreement. From what I can tell, the author is following a particular situation and he found a solution, but the changes can be implemented in a different way.

[Flyingmana]

Why to change array to ArrayObject?

However ... for ArrayObject append() should be used to add elements.

to make it manipulable via the observer I guess

[sreichel]

• arrays can also be changed in observers
• @kiatng comment?
• why not v19? Its no BC break.

[Flyingmana]

• why not v19? Its no BC break.

but its a new Feature, so its better in V20

However ... for ArrayObject append() should be used to add elements.

why does using append() make a difference here? the offsetSet of ArrayAccess should cover it, no?

[sreichel]

All new features have been added to v19 .... we already addes some new events to v19. Why to change it now?

(i`d reverted it to simple array ... makes no differences for observer and have less changes)

[Flyingmana]

All new features have been added to v19 .... we already addes some new events to v19. Why to change it now?

it happened because people ignored the "no new features into v19"

[sreichel]

Who said that? We added new features before v20 was created ... v20 was only for breaking changes???

[sreichel]

Did you read @kiatng suggestion?

[sreichel]

Will you update README too?

[fballiano]

This shouldn’t have been merged.

[Flyingmana]

It was useful, adding a new event is not very invasive, it had 3 approves(before fixing codestyle issues) and no change request.

[fballiano]

You switched the branch, made a modification and we were waiting on some feedbad. It shouldn’t have been merged!

[sreichel]

and no change request.

• it was set to draft
• Is that OK? Can you test to see if it works for you?
• i asked for change ArrayObject back to array
• i asked for adding this to v19

...

[Flyingmana]

I requested the change of the branch, the branch change was approved by to submitter, and technically @fballiano changed the branch.
The 3 Approves came afterwards.
The modifications were just codestyle changes and changing to equivalent methods.
The requested feedback points were no blocking reasons for these changes and more about evaluating alternatives, which all would have been more invasive and would not stand in conflict with this PR.

It was set to draft because of the merge conflict, which was resolved (and very straightforward)

And again, none of you did a formell change request blocking the merge.

[fballiano]

Because we don’t block other people’s work with change request like you do but ask questions instead. The questions came after colin’s review and you know it.

[fballiano]

And i switched the branch 7 months ago! Before Sven started this huge work we all see every day which also unifies the branches a bit to avoid the constant conflicts.

[sreichel]

but ask questions instead

And this worked very well. I cant remember any merged PR having open questions or suggestions ...

[fballiano]

but ask questions instead

And this worked very well. I cant remember any merged PR having open questions or suggestions ...

exactly, without frictions

[Flyingmana]

Because we don’t block other people’s work with change request like you do but ask questions instead. The questions came after colin’s review and you know it.

exactly, without frictions

And this worked very well. I cant remember any merged PR having open questions or suggestions ...

so here we are getting personal, having frictions and not talking anymore about the PR content.
Do you want to add something how the merged changes degrade the quality of the project, or cause any issues now or possibly in the future?

please move the other stuff into an own place, else I have to Lock here.

[fballiano]

you behave like this is your own property.

[sreichel]

Nailed it.

[fballiano]

so here we are getting personal, having frictions and not talking anymore about the PR content.

what are you talking about? we think the merge was a mistake, it's not getting personal. 3 maintainers didn't like this PR 100% and asked for modification and you (after months we didn't see you here) come, merge it enforcing it (since when you change a line you invalidated all reviews) and threaten to lock the discussion. this is not ok.

[Flyingmana]

• arrays can also be changed in observers

@sreichel @fballiano

https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/Mage.php#L503-L509

arrays are copy-on-write, without being explicitly passed by reference & there is no way the change is getting back here. Thats why the ArrayObject is needed, because objects are always pass-by-reference.

[sreichel]

Maybe you got me wrong?

You are right, it has to be passed by reference. This works too ...

Mage::dispatchEvent('eav_entity_attribute_frontend_get_class', ['classes' => &$out, 'attribute' => $this->getAttribute()]);

[Flyingmana]

huh, Iam surprised this works for parts of array, but having only a part of an array being a reference looks like a mean and easy to overlook pitfall for people who continue to pass these to other functions, while an arrayObject is very clear for this and making it more transparent.
What is the downside of an arrayObject?

<?php

function test ($data) {
    $data['classes'][] = "yes";
}

$out = ["does it?"];
$array = [
    "classes" => &$out,
    "attribute" => [ "value"=>"other"]
];
test($array);
var_dump($array);

array(2) {
  ["classes"]=>
  &array(2) {
    [0]=>
    string(8) "does it?"
    [1]=>
    string(3) "yes"
  }
  ["attribute"]=>
  array(1) {
    ["value"]=>
    string(5) "other"
  }
}

[sreichel]

What is the downside of an arrayObject?

No downsides. Only unnecessary changes. Btw ... it also used in core ...

        Mage::dispatchEvent('catalog_product_attribute_update_before', [
            'attributes_data' => &$attrData,
            'product_ids'   => &$productIds,
            'store_id'      => &$storeId
        ]);