Upstream merge 1.9.4.1 #639
Conversation
Magento 1.9.4.1
Conflicts: app/code/core/Mage/Compiler/Block/Process.php app/code/core/Mage/Compiler/Helper/Data.php app/code/core/Mage/Compiler/Model/Process.php app/code/core/Mage/Compiler/controllers/Adminhtml/Compiler/ProcessController.php app/code/core/Mage/Compiler/controllers/ProcessController.php app/code/core/Mage/Compiler/etc/adminhtml.xml app/code/core/Mage/Compiler/etc/compilation.xml app/code/core/Mage/Compiler/etc/config.xml app/code/core/Mage/Sales/doc/test.php app/design/adminhtml/default/default/layout/compiler.xml app/design/adminhtml/default/default/template/compiler/process.phtml app/etc/modules/Mage_Compiler.xml shell/compiler.php
There was a problem hiding this comment.
The issues mentioned in #602 are still present in this PR, so #602 should get merged first. The PR also introduces a new mismatch in lib/phpseclib/bootstrap.php which is not covered by #602. Looks like an issue with how this PR was created.
Issues that popped up during the usual rebase procedure in my fork:
- only 2 minor merge conflicts related to copyright header changes in deleted files
Mage_Compilerwas removed in1.9.4.1, but not as thoroughly as our related commit- Wishlist observer regression from
1.9.4.0is not fixed in1.9.4.1
To look at the details clone https://github.com/edannenberg/magento-lts and do a directory compare with this PR. Note that 1.9.4.x still has a couple of missing commits from 1.9.3.x that are already in my fork.
|
the bootstrap mismatch did already happen on the 1.9.4.0 import(I probably chose the wrong side for the conflict. So I would push this into a separate PullRequest afterwards I merged #602 I did forward port the current changes in 1.9.3.x to 1.9.4.x via #645 I remember looking into the wishlist observer thing and discussing it in a slack, sadly the history there is already lost, and it was not yet discussed here in the repository. The change was introduced by the PHP 7.2 patch, but was not included into official releases, so Iam not sure if it can be called a regression. Maybe, as it is about multiple wishlists, it was not meant to be for CE and just accidentally made it into the patch back then. (just writing it down here, so Iam not so confused about this topic the next time) Anything else I missed? |
Yea looks like I missed that one when creating #602.
I took another look at it, pretty sure this is a regression. The relevant PHP7.2 patch change: -if (count($wishlistIds) && $request->getParam('wishlist_next')){
+if (!empty($wishlistIds) && $request->getParam('wishlist_next')){Then in 1.9.4.0 this changed to: -if (!empty($wishlistIds) && $request->getParam('wishlist_next')){
+if (empty($wishlistIds) && $request->getParam('wishlist_next')){
The regression never made it into this repo when 1.9.4.0 was merged, so right now no further actions are needed.
Looked good otherwise from my end. You can rebase the PR branch and do another directory compare with my fork, if both match everything is in order. |
| ); | ||
| $logValidator = new Zend_Validate_File_Extension($_allowedFileExtensions); | ||
| $logDir = self::getBaseDir('var') . DS . 'log'; | ||
| if (!$logValidator->isValid($logDir . DS . $file)) { |
There was a problem hiding this comment.
Not sure about to change this ... see #646
There was a problem hiding this comment.
Another patch another regression, le sigh. Should get addressed in a separate PR though.
There was a problem hiding this comment.
After digging a bit into the issue the code is actually correct, for some reason Zend_Validate_File_Extension::isValid() also does a file check, which really is none of it's business.
There was a problem hiding this comment.
Not said its wrong. Finally it does nearly the same as the old check - if the file check is removed :)
as there were issues with the last merge from the mirror, I do provide it now as a PR for an extra review