Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed app/code/core/Zend #2861

Merged
merged 4 commits into from
Jan 16, 2023
Merged

Conversation

fballiano
Copy link
Contributor

Since the inclusion of ZF1Future the files in app/code/core/Zend are not correctly loaded anymore.
Porting the modifications into ZF1 via composer patch seems not possible because ZF1's code is now different and the patch doesn't seem to be compatible anymore.

The reason for this PR is explained in this comment: #2857 (comment)

Related Issues

  1. Rebuild Tag aggregation data not working #2857

@ADDISON74
Copy link
Collaborator

ADDISON74 commented Dec 29, 2022

I renamed the /app/code/core/Zend directory then I checked the issue reported here #2237. I was able to reproduce it.

Just visit Backend > Reports > Reviews > Products Reviews. Here is the report file content

a:5:{i:0;s:1443:"SELECT `e`.*, (SELECT COUNT(DISTINCT rev.review_id) FROM `review` AS `rev` WHERE (e.entity_id = rev.entity_pk_value)) AS `review_cnt`, MAX(r.created_at) AS `last_created`, `SUM(``table_rating``.``percent``)/COUNT(table_rating`.`rating_id)` AS `avg_rating`, `SUM(table_rating.percent_approved)/COUNT(table_rating`.`rating_id)` AS `avg_rating_approved` FROM `catalog_product_entity` AS `e`
 INNER JOIN `review` AS `r` ON e.entity_id = r.entity_pk_value
 LEFT JOIN `rating_option_vote_aggregated` AS `table_rating` ON e.entity_id = table_rating.entity_pk_value AND table_rating.store_id > 0 GROUP BY `e`.`entity_id` ORDER BY `review_cnt` DESC LIMIT 20

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'SUM(`table_rating`.`percent`)/COUNT(table_rating.rating_id)' in 'field list', query was: SELECT `e`.*, (SELECT COUNT(DISTINCT rev.review_id) FROM `review` AS `rev` WHERE (e.entity_id = rev.entity_pk_value)) AS `review_cnt`, MAX(r.created_at) AS `last_created`, `SUM(``table_rating``.``percent``)/COUNT(table_rating`.`rating_id)` AS `avg_rating`, `SUM(table_rating.percent_approved)/COUNT(table_rating`.`rating_id)` AS `avg_rating_approved` FROM `catalog_product_entity` AS `e`
 INNER JOIN `review` AS `r` ON e.entity_id = r.entity_pk_value
 LEFT JOIN `rating_option_vote_aggregated` AS `table_rating` ON e.entity_id = table_rating.entity_pk_value AND table_rating.store_id > 0 GROUP BY `e`.`entity_id` ORDER BY `review_cnt` DESC LIMIT 20";i:1;s:5169:"#0 /var/www/html/lib/Varien/Db/Statement/Pdo/Mysql.php(104): Zend_Db_Statement_Pdo->_execute(Array)
#1 /var/www/html/vendor/shardj/zf1-future/library/Zend/Db/Statement.php(303): Varien_Db_Statement_Pdo_Mysql->_execute(Array)
#2 /var/www/html/vendor/shardj/zf1-future/library/Zend/Db/Adapter/Abstract.php(480): Zend_Db_Statement->execute(Array)
#3 /var/www/html/vendor/shardj/zf1-future/library/Zend/Db/Adapter/Pdo/Abstract.php(238): Zend_Db_Adapter_Abstract->query('SELECT `e`.*, (...', Array)
#4 /var/www/html/lib/Varien/Db/Adapter/Pdo/Mysql.php(492): Zend_Db_Adapter_Pdo_Abstract->query('SELECT `e`.*, (...', Array)
#5 /var/www/html/vendor/shardj/zf1-future/library/Zend/Db/Adapter/Abstract.php(739): Varien_Db_Adapter_Pdo_Mysql->query('SELECT `e`.*, (...', Array)
#6 /var/www/html/lib/Varien/Data/Collection/Db.php(757): Zend_Db_Adapter_Abstract->fetchAll('SELECT `e`.*, (...', Array)
#7 /var/www/html/app/code/core/Mage/Eav/Model/Entity/Collection/Abstract.php(1057): Varien_Data_Collection_Db->_fetchAll('SELECT `e`.*, (...')
#8 /var/www/html/app/code/core/Mage/Eav/Model/Entity/Collection/Abstract.php(876): Mage_Eav_Model_Entity_Collection_Abstract->_loadEntities(false, false)
#9 /var/www/html/app/code/core/Mage/Adminhtml/Block/Widget/Grid.php(605): Mage_Eav_Model_Entity_Collection_Abstract->load()
#10 /var/www/html/app/code/core/Mage/Adminhtml/Block/Report/Review/Product/Grid.php(46): Mage_Adminhtml_Block_Widget_Grid->_prepareCollection()
#11 /var/www/html/app/code/core/Mage/Adminhtml/Block/Widget/Grid.php(708): Mage_Adminhtml_Block_Report_Review_Product_Grid->_prepareCollection()
#12 /var/www/html/app/code/core/Mage/Adminhtml/Block/Widget/Grid.php(719): Mage_Adminhtml_Block_Widget_Grid->_prepareGrid()
#13 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(931): Mage_Adminhtml_Block_Widget_Grid->_beforeToHtml()
#14 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(650): Mage_Core_Block_Abstract->toHtml()
#15 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(594): Mage_Core_Block_Abstract->_getChildHtml('grid', true)
#16 /var/www/html/app/code/core/Mage/Adminhtml/Block/Widget/Grid/Container.php(92): Mage_Core_Block_Abstract->getChildHtml('grid')
#17 /var/www/html/app/design/adminhtml/default/default/template/widget/grid/container.phtml(30): Mage_Adminhtml_Block_Widget_Grid_Container->getGridHtml()
#18 /var/www/html/app/code/core/Mage/Core/Block/Template.php(257): include('/var/www/html/a...')
#19 /var/www/html/app/code/core/Mage/Core/Block/Template.php(291): Mage_Core_Block_Template->fetchView('adminhtml/defau...')
#20 /var/www/html/app/code/core/Mage/Core/Block/Template.php(304): Mage_Core_Block_Template->renderView()
#21 /var/www/html/app/code/core/Mage/Adminhtml/Block/Template.php(74): Mage_Core_Block_Template->_toHtml()
#22 /var/www/html/app/code/core/Mage/Adminhtml/Block/Widget/Container.php(300): Mage_Adminhtml_Block_Template->_toHtml()
#23 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(932): Mage_Adminhtml_Block_Widget_Container->_toHtml()
#24 /var/www/html/app/code/core/Mage/Core/Block/Text/List.php(42): Mage_Core_Block_Abstract->toHtml()
#25 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(932): Mage_Core_Block_Text_List->_toHtml()
#26 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(650): Mage_Core_Block_Abstract->toHtml()
#27 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(594): Mage_Core_Block_Abstract->_getChildHtml('content', true)
#28 /var/www/html/app/design/adminhtml/default/default/template/page.phtml(69): Mage_Core_Block_Abstract->getChildHtml('content')
#29 /var/www/html/app/code/core/Mage/Core/Block/Template.php(257): include('/var/www/html/a...')
#30 /var/www/html/app/code/core/Mage/Core/Block/Template.php(291): Mage_Core_Block_Template->fetchView('adminhtml/defau...')
#31 /var/www/html/app/code/core/Mage/Core/Block/Template.php(304): Mage_Core_Block_Template->renderView()
#32 /var/www/html/app/code/core/Mage/Adminhtml/Block/Template.php(74): Mage_Core_Block_Template->_toHtml()
#33 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(932): Mage_Adminhtml_Block_Template->_toHtml()
#34 /var/www/html/app/code/core/Mage/Core/Model/Layout.php(580): Mage_Core_Block_Abstract->toHtml()
#35 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Action.php(397): Mage_Core_Model_Layout->getOutput()
#36 /var/www/html/app/code/core/Mage/Adminhtml/controllers/Report/ReviewController.php(91): Mage_Core_Controller_Varien_Action->renderLayout()
#37 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Action.php(428): Mage_Adminhtml_Report_ReviewController->productAction()
#38 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(262): Mage_Core_Controller_Varien_Action->dispatch('product')
#39 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Front.php(188): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http))
#40 /var/www/html/app/code/core/Mage/Core/Model/App.php(371): Mage_Core_Controller_Varien_Front->dispatch()
#41 /var/www/html/app/Mage.php(745): Mage_Core_Model_App->run(Array)
#42 /var/www/html/index.php(71): Mage::run('', 'store')
#43 {main}";s:3:"url";s:76:"/index.php/admin/report_review/product/key/114c1f85a15f11a74c23b48e4cc1d4a9/";s:11:"script_name";s:10:"/index.php";s:4:"skin";s:5:"admin";}

It seems that there is an error in the OM code similar to the one solved yesterday with the Tags module. Maybe we should do a search in the source code for such database queries and if we find something to change them.

@fballiano
Copy link
Contributor Author

checking it now

@fballiano
Copy link
Contributor Author

@ADDISON74 it should be fixed by #2864 ;-)

@ADDISON74
Copy link
Collaborator

Good job. I confirm that the issue was fixed.

Couldn't we look for similar situations in the OpenMage source code? It is obvious that if we remove those two files we could have issues with querying the database. I am thinking of a search using a REGEX but all possible phrases must be identified. So far we identified COUNT, MAX in the queries. It would be helpful if someone knows what we should be looking for.

Obviously, those two files must be removed from OM but I will approve the PR only after we make sure that we have done more comprehensive checks in order not to create BC situations. At least a solution format exists so far in the way two reported issues were solved, one here #2858 and another here #2864.

Those two files that we want to delete are related to working with the database. From what I can see the issues in the OM code could appear in modules in the directory /Model/Resource/. So far, two files have been modified, both named Collection.php.

@fballiano
Copy link
Contributor Author

mmm it's not easy cause it's not about the count or max, but when they are combined together in multiple function calls :-(

@fballiano
Copy link
Contributor Author

this files are not used anyway now, the autoloader doesn't load them

@elidrissidev
Copy link
Member

elidrissidev commented Dec 29, 2022

this files are not used anyway now, the autoloader doesn't load them

they are loaded when installing OM via composer. That means whatever the issue that's causing this, it will also prevent overriding any Zend file by putting it inside app/code/core, which I saw some extensions do this for better or worse.

@ADDISON74
Copy link
Collaborator

ADDISON74 commented Dec 29, 2022

It is possible that we do not need to make changes everywhere. I did a search in the OM source code for the following strings

=> 'COUNT(
=> 'MAX(
=> 'SUM(
=> "COUNT(
=> "MAX(
=> "SUM(

and I noticed that there are many files where we find $this->getSelect()->columns(). I started visiting the sections in Frontend/Backend where these files are used. So far I haven't gotten any errors. If I still find errors, I will create a post in the Issues section where we can group them all together.

@fballiano
Copy link
Contributor Author

A fix is really necessary only when the conditions are combined together, like with a mathematical operation, otherwise everything is working fine 😊

ADDISON74
ADDISON74 previously approved these changes Dec 30, 2022
@sreichel
Copy link
Contributor

A fix is really necessary only when the conditions are combined together, like with a mathematical operation, otherwise everything is working fine blush

What about 3rd code that uses similiar code?

they are loaded when installing OM via composer

I admit to not having tested this case and found further issues.

  1. patches are not applied. You need to add this to your composer.json
  2. if you have a custom magento-root-dir like htdocs patch files are not found. (PR incoming)

@fballiano
Copy link
Contributor Author

About 3rd party code: it was working kinda by mistake, ZF was always clear about that, expressions have to be quoted in Zend_Db_Expr.

At the same time I'll put it in another way: what about the security updates (maybe, I don't really know) in ZF1F that we're neglecting if we don't remove these 2 files?

@elidrissidev elidrissidev added the backwards compatibility Might affect backwards compatibility for some users label Dec 31, 2022
@ADDISON74
Copy link
Collaborator

ADDISON74 commented Jan 13, 2023

After a week of use I did not get any error. I did almost everything that could be done in Frontend and Backend. Please do the same thing and let's move forward when this PR gets 3 approvals.

@fballiano
Copy link
Contributor Author

Thanks @ADDISON74

@sreichel sreichel self-assigned this Jan 15, 2023
@fballiano fballiano merged commit 7302b5e into OpenMage:1.9.4.x Jan 16, 2023
@fballiano fballiano deleted the removed_zend_override branch January 16, 2023 11:13
fballiano added a commit that referenced this pull request Jan 16, 2023
@fballiano
Copy link
Contributor Author

merged and cherrypicked to v20!

justinbeaty pushed a commit to justinbeaty/magento-lts that referenced this pull request Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards compatibility Might affect backwards compatibility for some users documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants