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

Import Magento Release 1.9.3.0 #116

Closed
drobinson opened this issue Oct 13, 2016 · 15 comments
Closed

Import Magento Release 1.9.3.0 #116

drobinson opened this issue Oct 13, 2016 · 15 comments

Comments

@drobinson
Copy link
Collaborator

Hi @OpenMage/lts-admins

I've created the new branch for the 1.9.3 release after importing the diff from OpenMage/magento-mirror@d48bebc

There were some patch conflicts - surprisingly most of them were related to patches we've already applied for PHP 7 support (although Magento does not claim compatibility with PHP 7 with this release). Regardless, I've listed the files I encountered patch conflicts with below:

.//app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php.rej
.//app/code/core/Mage/Core/Model/Layout.php.rej
.//app/code/core/Mage/ImportExport/Model/Export/Entity/Customer.php.rej
.//app/code/core/Mage/ImportExport/Model/Export/Entity/Product/Type/Abstract.php.rej
.//app/code/core/Mage/ImportExport/Model/Import/Uploader.php.rej
.//app/code/core/Mage/Sales/etc/config.xml.rej
.//app/code/core/Mage/Tax/etc/config.xml.rej
.//app/locale/en_US/Mage_Rss.csv.rej
.//lib/Varien/File/Uploader.php.rej

Please review what I ended up with and compare it both with the history of this repo and the official release. If you approve of the resolution I came up with add +1 to this issue. If you have any problems (with these files or any others), please outline them here and I'll work on resolving them in the new branch.

Once we have +3 or so I'll update the default branch on this repo to 1.9.3 (effectively releasing it). Sorry we can't do this in a PR - I don't think you can submit a PR for a new branch...

@alexkirsch
Copy link
Contributor

alexkirsch commented Oct 13, 2016

David,

I noticed you committed app\code\core\Mage\Catalog\Model\Resource\Product\Type\Configurable\Attribute\Collection.php.orig

It also appears that PR-72 (commit: ac1e3a7) was merged into this branch. Which caused the conflict, from a quick code review the official code now supports proper sorting so we could revert PR-72 completely.

@alexkirsch
Copy link
Contributor

Good on all the other files changed (except for the other .orig files)

@drobinson
Copy link
Collaborator Author

drobinson commented Oct 13, 2016

@alexkirsch thanks for pointing this out. I removed the .orig files, but just to make sure I'm not going crazy, does this file look correct to you (https://github.com/OpenMage/magento-mirror/commits/magento-1.9/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php)? It seems Magento incorporated the change from PR-70 (commit: ac1e3a7) in their official release.

@drobinson
Copy link
Collaborator Author

Merge conflict you found is resolved in a25c721

@alexkirsch
Copy link
Contributor

alexkirsch commented Oct 13, 2016

@davidwindell You have the uasort inside the foreach.. which would still work it would simply sort more times than necessary.

I would bring it back in line with upstream

@alexkirsch
Copy link
Contributor

Also in the future, I would create an empty branch, and create a PR against the new branch vs doing it as an issue.

@drobinson drobinson changed the title Import Magento Release 1.9.3 Import Magento Release 1.9.3.0 Oct 13, 2016
@drobinson
Copy link
Collaborator Author

@alexkirsch Good idea RE: empty branch. We should be in line with upstream on this now.

@davidwindell
Copy link
Contributor

@alexkirsch did you mean me?

@alexkirsch
Copy link
Contributor

@davidwindell no, sorry about that.. I should blame the auto complete :)

@Flyingmana
Copy link
Contributor

Iam very sorry, I will not be able to do a proper review before monday.
In case someone else is reviewing this positively, remember to change the default branch.

@drobinson
Copy link
Collaborator Author

Can anyone else take a look at this so I can move forward with making 1.9.3.0 the default branch? We have a PR for removing the swf files that have been sticking around since we moved to the new process.

@Flyingmana
Copy link
Contributor

so, finally did my review.
To actually verify the endresult I took the other route to apply all patches again on the magento-mirror 1.9.3.0 version (which I also verified before)

you can checkout it on https://github.com/Flyingmana/magento-lts/tree/1.9.3.0-based
It uses a mix of cherry-picks, merge "own" and normal merges.

I had two times a merge conflict.

Once for the PHP7 patch, which added most, but did not add ',msrp' to the shipping>after config. (not sure if its important, but probably?)
another one for https://github.com/OpenMage/magento-lts/pull/84/files
here I actually came to a different result https://github.com/Flyingmana/magento-lts/blob/1.9.3.0-based/app/code/core/Mage/Tax/etc/config.xml

                    <tax_subtotal>
                        <class>tax/sales_total_quote_subtotal</class>
                        <after>subtotal,nominal,shipping,freeshipping</after>
                        <before>tax,discount,msrp</before>
                    </tax_subtotal>

Oh, and I also did a diff in general between my and your branch.
the main part:

diff --git a/app/code/core/Mage/Tax/etc/config.xml b/app/code/core/Mage/Tax/etc/config.xml
index 3c6b038..320c6c8 100644
--- a/app/code/core/Mage/Tax/etc/config.xml
+++ b/app/code/core/Mage/Tax/etc/config.xml
@@ -162,7 +162,7 @@
                 <totals>
                     <tax_subtotal>
                         <class>tax/sales_total_quote_subtotal</class>
-                        <after>subtotal</after>
+                        <after>subtotal,nominal,shipping,freeshipping</after>
                         <before>tax,discount,msrp</before>
                     </tax_subtotal>
                     <tax_shipping>
diff --git a/cron.sh b/cron.sh
old mode 100755
new mode 100644
diff --git a/mage b/mage
old mode 100755
new mode 100644
diff --git a/skin/adminhtml/default/default/media/flex.swf b/skin/adminhtml/default/default/media/flex.swf
deleted file mode 100644
index a8ecaa0..0000000
Binary files a/skin/adminhtml/default/default/media/flex.swf and /dev/null differ
diff --git a/skin/adminhtml/default/default/media/uploader.swf b/skin/adminhtml/default/default/media/uploader.swf
deleted file mode 100644
index e38a5a5..0000000
Binary files a/skin/adminhtml/default/default/media/uploader.swf and /dev/null differ
diff --git a/skin/adminhtml/default/default/media/uploaderSingle.swf b/skin/adminhtml/default/default/media/uploaderSingle.swf
deleted file mode 100644
index 3dd31ce..0000000
Binary files a/skin/adminhtml/default/default/media/uploaderSingle.swf and /dev/null differ
diff --git a/skin/frontend/base/default/images/window_overlay.png b/skin/frontend/base/default/images/window_overlay.png
new file mode 100644
index 0000000..681a61d
Binary files /dev/null and b/skin/frontend/base/default/images/window_overlay.png differ
diff --git a/skin/frontend/default/blank/images/window_overlay.png b/skin/frontend/default/blank/images/window_overlay.png
new file mode 100644
index 0000000..681a61d
Binary files /dev/null and b/skin/frontend/default/blank/images/window_overlay.png differ
diff --git a/skin/frontend/default/blue/images/window_overlay.png b/skin/frontend/default/blue/images/window_overlay.png
new file mode 100644
index 0000000..681a61d
Binary files /dev/null and b/skin/frontend/default/blue/images/window_overlay.png differ
diff --git a/skin/frontend/default/default/images/window_overlay.png b/skin/frontend/default/default/images/window_overlay.png
new file mode 100644
index 0000000..681a61d
Binary files /dev/null and b/skin/frontend/default/default/images/window_overlay.png differ
diff --git a/skin/frontend/default/modern/images/window_overlay.png b/skin/frontend/default/modern/images/window_overlay.png
new file mode 100644
index 0000000..681a61d
Binary files /dev/null and b/skin/frontend/default/modern/images/window_overlay.png differ
diff --git a/var/package/Interface_Adminhtml_Default-1.9.1.0.xml b/var/package/Interface_Adminhtml_Default-1.9.1.0.xml
deleted file mode 100644
index d9b067f..0000000

and then a list of package xml files related to "1.9.1.0" which should not exist anymore

Iam not sure about the file modes if I broke something, or if they are wrong in your branch.

But as a whole, I would say we can switch to use your branch as default

@infabo
Copy link

infabo commented Oct 19, 2016

I did a quick diff myself too, but I did not apply the PHP7 patch.

So basically my spottings with a look at #62:

  • window_overlay.pngs were added
  • swfs should be removed
  • old package xmls should be removed

It's quite astonishing what bugs/typos still exist in official 1.9.3.0....

@seansan
Copy link
Contributor

seansan commented Oct 24, 2016

Are we planning on labelling/tagging this release once we fully merge? I was looking at 1 of our test sites today to maybe give it a spin and download full 1.9.3.0 from the repo.

About labelling
Also are we then going to label it as: 1.9.3.0.x???? Where X is an incremental number? We could "package" some fixes/improvements - make it a release like 1.9.3.0.1 for the first one ... after that for every xyz bug fixes we add a release 1.9.3.0.2 1.9.3.0.3 etc ... then when 1.9.4.0 comes out or 1.9.3.1 for that matter we continue as 1.9.3.0.2 1.9.3.0.3 1.9.3.1.4 1.9.3.1.5 (or 1.9.4.0.4 1.9.4.0.5 - this way we know what "version" LTS updates we are on: ie the last number)

@drobinson
Copy link
Collaborator Author

@infabo The second two points are fixed in PR's #121 #120

@seansan We had discussed that in the past, but decided against it due to how Magento was versioning thier releases (and not coming out with new releases for patches). With the new release pattern Magento is following, that makes more sense - here's a new issue we can use to have a discussion about it: #125

It looks like 1.9.3.0 is approved by a good # of people, so I'll make that the default branch now and we can talk about tagging it in that discussion. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants