Skip to content

Fix currency validator on BE. Throw error when value is empty. #16222

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix currency validator on BE. Throw error when value is empty. #16222

wants to merge 2 commits into from

Conversation

bmxmale
Copy link

@bmxmale bmxmale commented Jun 18, 2018 •
edited
Loading

Description

currency-validator

Add statement to ignore validation when value is empty.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jun 18, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @bmxmale. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on Pull Request changes
  • @magento-engcom-team give me new test instance - deploy NEW test instance based on Pull Request changes
  • @magento-engcom-team give me {$VERSION} instance - deploy Vanilla Magento instance for Issue or Pull Request

For more details, please, review the Magento Contributor Assistant documentation

@phoenix128 phoenix128 self-assigned this Jun 22, 2018
@phoenix128
Copy link
Contributor

Hello @bmxmale ,
thank you for your contribution.

Would you please provide a scenario to reproduce this issue?
Thanks.

@bmxmale
Copy link
Author

bmxmale commented Jun 24, 2018 •
edited
Loading

Hi @phoenix128,
sorry for delay in response.

On Stores -> Configuration -> General -> Currency Setup
Setup Base Currency, Default Display Currency, Allowed Currencies to eg Danish Krone

Next prepare bin/magento app:config:dump

When magento read data from config.php file some backend validations like this from PR get empty values so throwing errors.

I also update to latest 2.2-develop version. Problem still persist.

@phoenix128
Copy link
Contributor

I confirm this issue, but I am not sure this is the way to fix it.
I think we should skip the wole validation procedure for such fields coming from config.php.
I will start an internal discussion about that. Meanwhile I am putting this on hold.

@bmxmale
Copy link
Author

bmxmale commented Jun 25, 2018

@phoenix128 I agree to resolve problem on top level but fix for that could take some time. My fix don't mess much, but provide possibility to save config on this case.

When you finish internal discussion I could help with fixing config from file case.

@phoenix128
Copy link
Contributor

I understand @bmxmale ,
we will handle this as soon as possible. I'll do my best to not delay it.

@phoenix128
Copy link
Contributor

phoenix128 commented Jun 25, 2018 •
edited
Loading

Hello @bmxmale ,
we have been talking about this and we agree that this PR is just trying to cover the issue and it may also have some unexpected behaviour.

The main problem seems to be in \Magento\Config\Model\Config::_processGroup and seems to be system wide. Not only currency related.

We are working to find a more complete solution unless you want to try to provide it.

phoenix128 added a commit to phoenix128/magento2 that referenced this pull request Jun 25, 2018
@phoenix128
Rework for PR magento#16222 .
ab3f767 
When running 'app:config:dump', admin fields get disabled, but clicking save will process them anyway
This behaivour causes a validation check on empty fields since the admin form fields are disabled.
This commit checks when the field is read only and skips its save.
@phoenix128 phoenix128 mentioned this pull request Jun 25, 2018
Merged
4 tasks
@phoenix128
Copy link
Contributor

Hello @bmxmale ,
I opened another PR with a more complete solution here: #16393 .

You found a very important issue, thank you very much for your time and for your work.

@phoenix128 phoenix128 closed this Jun 25, 2018
magento-engcom-team pushed a commit that referenced this pull request Jul 7, 2018
ENGCOM-2127: Rework for PR #16222 . #16393
7be5373
magento-engcom-team pushed a commit that referenced this pull request Jul 7, 2018
🔃 [EngCom] Public Pull Requests - 2.2-develop
d91c0c0 
Accepted Public Pull Requests:
 - #16576: [Backport] Declare module namespace before template path name (by @mageprince)
 - #16557: Updated SynonymGroup.xml (by @sanganinamrata)
 - #16553: Update mini-cart checkout translations (by @JeroenVanLeusden)
 - #16549: Corrected function comment (by @sanganinamrata)
 - #16524: Clear converted file data  (by @gelanivishal)
 - #16517: Fix responsive tables showing broken heading (by @LordZardeck)
 - #16393: Rework for PR #16222 . (by @phoenix128)
 - #16090: Added and removed unnecessary translation for label/comment tags (by @yogeshks)
 - #16581: Removed double occurrences from files. (by @sanganinamrata)
 - #15909: [Backport] Fix for Wrong price amount on product page (by @gelanivishal)


Fixed GitHub Issues:
 - #11717: Wrong price amount on product page (reported by @HirokazuNishi) has been fixed in #15909 by @gelanivishal in 2.2-develop branch
   Related commits:
     1. ea16687
     2. 8a4d966
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Jul 26, 2018
2.3 Rework for PR magento#16222
8f31dde
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Aug 3, 2018
2.3 Rework for PR magento#16222: added empty line
b1700f7
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Aug 3, 2018
2.3 Rework for PR magento#16222: changed property access type
fadb8a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@phoenix128 phoenix128

Labels
Component: Config Progress: on hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bmxmale @magento-cicd2 @magento-engcom-team @phoenix128