Move orig data to abstract model (according to Magento 2.x)

[Sekiphp]

Description (*)

Move origData property and related methods to mage_Core_Model_Abstract according to Magento 2.x.

Related M2 file: https://github.com/magento/magento2/blob/fbfac3e51730abe2b79c6072544bc9742fcf7606/lib/internal/Magento/Framework/Model/AbstractModel.php#L56

These methods are not used in any direct child of Varien_Object:
$ grep -R -B10000 -A10000 -P 'OrigData|dataHasChangedFor' . | grep -F "extends Varien_Object"

Output:
./app/code/core/Mage/Core/Model/Abstract.php-abstract class Mage_Core_Model_Abstract extends Varien_Object

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)

[Sekiphp]

Opened PR to another branch @midlan @Flyingmana @kkrieger85

#1322

[colinmollenhour]

While we're refactoring code why not put it in a trait so that if someone needs to use it in a class that extends Varien_Object but not Mage_Core_Model_Abstract it can be easily added?

[midlan]

@colinmollenhour Why not? Let me explain.

From my view, the origData is strongly linked to object loading. Even the doc says it is linked to object loading:

Get object loaded data (original data)

Nowadays I assume there is simply no programmer who would like reinvent new own object model/orm and use such trait, since there are plenty of object models already invented laying on github.

Magento 2 team is thinking the same (the didn't created the trait). And there does not exist such feature request in M2.

Another thing is that once you divide the class into class + trait, you must keep the back compatibility of both of them the whole life of openmage major version. I would divide it only in the case someone will send feature request that he/she needs it (what probably won't happen as I written above). In other words: do not make things complicated, only because someone may need it.

I am doing the same. Every pull request I open is bugfix or thing that I really run into or used in my Magento/OpenMage instance.

[joshua-bn]

@midlan makes a very good point... don't add features because someone might want them.

[colinmollenhour]

I'm fine with it as is, just thinking that if in the unlikely case that this does break BC for someone who used _origData on an instance of Varien_Object that did not extend Mage_Core_Model_Abstract the remedy would be to simply add the trait rather than copy each method over to their custom model.

Edit: trait name suggested would be something like Varien_Object_OrigDataTrait

[midlan]

@Flyingmana correctly changed the base branch to 20.x so the BC break is acceptable during major upgrade.

In the case it really breaks something, I would first recommend to change the parent to Mage_Core_Model_Abstract if applicable. If not, the trait is way.

[github-actions]

Unit Test Results

1 files  1 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
2 runs  2 ✔️ 0 💤 0 ❌

Results for commit 42988a6.

[fballiano]

I think this was a mistake, how do we solve the phpstan error on #2206 https://github.com/OpenMage/magento-lts/runs/6803200822?check_suite_focus=true ? that are some parts of the core that are calling dataHasChangedFor() but they can expect Varien_Object objects.

What are we doing now with those?

[kiatng]

There are 3 errors:

  ------ ----------------------------------------------------------------- 
  Line   Catalog/Model/Category/Indexer/Product.php                       
 ------ ----------------------------------------------------------------- 
  199    Call to an undefined method Varien_Object::dataHasChangedFor().  
  200    Call to an undefined method Varien_Object::dataHasChangedFor().  
 ------ ----------------------------------------------------------------- 
 ------ ----------------------------------------------------------------- 
  Line   Catalog/Model/Resource/Product/Attribute/Backend/Urlkey.php      
 ------ ----------------------------------------------------------------- 
  66     Call to an undefined method Varien_Object::dataHasChangedFor().  
 ------ ----------------------------------------------------------------- 

For the first two,

magento-lts/app/code/core/Mage/Catalog/Model/Category/Indexer/Product.php

Lines 190 to 202 in c83207b

	protected function _registerProductEvent(Mage_Index_Model_Event $event)
	{
	$eventType = $event->getType();
	if ($eventType == Mage_Index_Model_Event::TYPE_SAVE) {
	/** @var Varien_Object|Mage_Catalog_Model_Product $product */
	$product = $event->getDataObject();
	/**
	* Check if product categories data was changed
	*/
	if ($product->getIsChangedCategories() || $product->dataHasChangedFor('status')
	|| $product->dataHasChangedFor('visibility') || $product->getIsChangedWebsites()) {
	$event->addNewData('category_ids', $product->getCategoryIds());
	}

Line 194 should be

             /** @var Mage_Catalog_Model_Product $product */ 

For the third error, the file is deprecated:

magento-lts/app/code/core/Mage/Catalog/Model/Resource/Product/Attribute/Backend/Urlkey.php

Lines 34 to 36 in c83207b

	* @deprecated since 1.7.0.2
	*/
	class Mage_Catalog_Model_Resource_Product_Attribute_Backend_Urlkey extends Mage_Eav_Model_Entity_Attribute_Backend_Abstract

That, I am not sure what can be done.

[edit] I think the last file's $object refers to Mage_Catalog_Model_Product, so the doc block at line 61 can be fixed accordingly.