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

Sync v19 v20 #2810

Merged
merged 2 commits into from
Dec 16, 2022
Merged

Sync v19 v20 #2810

merged 2 commits into from
Dec 16, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 15, 2022

Description (*)

Some minor changes to reduce differences ... (marked same methods as deprecated).

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)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: Core Relates to Mage_Core Component: Page Relates to Mage_Page Component: Sales Relates to Mage_Sales translations Relates to app/locale labels Dec 15, 2022
@fballiano fballiano merged commit a1ccc3a into 1.9.4.x Dec 16, 2022
@fballiano fballiano deleted the sync-v19-v20 branch December 16, 2022 09:28
@fballiano
Copy link
Contributor

every some time I've to say this, when will we get rid of v19? the differences aren't even that big to justify all the time put into fixing conflicts.

@sreichel
Copy link
Contributor Author

Then better ged rid of v20.

I diffed v19/v20 yesterday and found some changes that should be reverted (imho) to have less differences. Other could possibly backported,

(Btw ... this should be merged to v20 too.)

@fballiano
Copy link
Contributor

why do everybody hate v20? who needs ie8 and those iphone themes anymore?

@fballiano fballiano mentioned this pull request Dec 16, 2022
@sreichel
Copy link
Contributor Author

For me its not about IE support or themes. Just want to have as less differences as possible.

@fballiano
Copy link
Contributor

if we have zero differences is way better

@sreichel
Copy link
Contributor Author

Then we dont need v20 at all :P

@fballiano
Copy link
Contributor

if we backport everything v19 will be v20

@sreichel
Copy link
Contributor Author

Actually we cant backport everything (in one step).

Think there are some changes that need reviews.

@fballiano
Copy link
Contributor

fballiano commented Dec 16, 2022

git merge 20.0... :-D

I think some of the other maintainers won't be happy

@fballiano fballiano mentioned this pull request Jan 3, 2023
@@ -642,7 +642,7 @@ public function validateUploadFile($filePath)
}

if ($imageInfo[0] > $maxDimension || $imageInfo[1] > $maxDimension) {
Mage::throwException($this->__('Disalollowed file format.'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably breaks several translations and should have only get changed in the translation file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been arguing for the same thing for long time and I was alone saying that we should only update the translation files. At that time literally everybody was against my (and now your) idea. So please be more vocal next time. The people decided that they want to keep in sync strings in php files and in CSV files so now it is what it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that line it is about an exception that will be written in a log file. I think that these errors can be corrected in the PHP files and if they exist (although I doubt it) in the CSV files too. A programmer should know English, as well as a system administrator who has access to these files. On the other hand, although I do not agree to modify only the CSV files, I accepted PRs where such modifications were made, because it is for user interface and we should take care of translation without requiring efforts from those who periodically update the source code..

There are discussions on this topic in which the administrator takes note of the changes from one version to another and he makes the translations. From the programmer's perspective, honestly speaking, I don't like to see inconsistencies between sentences, especially when I look into the source code. I see something in the interface in English then in the code it's completely different or worse spelling mistakes like the one above, extremely annoying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important thing is that we do not keep discussing the same things over and over again. I'm fine with the decision taken although it was not the choice I'd love, sometime it's a personal taste and it is fine :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. i always said change only translation string in csv
  2. @Flyingmana this PR was just to get rid of differences between branches. If we change it back, we should also revert in v20.

@Flyingmana
Copy link
Contributor

@sreichel @fballiano if you want a change in supported versions: here is the place to contribute them OpenMage/rfcs#7

@Flyingmana Flyingmana added the backwards compatibility Might affect backwards compatibility for some users label Jan 3, 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 Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: Core Relates to Mage_Core Component: Page Relates to Mage_Page Component: Sales Relates to Mage_Sales translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants