Skip to content

Improve address save flow to allow to use custom validators #7882

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

Closed

Conversation

vovayatsyuk
Copy link
Member

@vovayatsyuk vovayatsyuk commented Dec 19, 2016

This is a copy of my previous PR: #7552. I've made some very bad things there, and I had to restart PR 😞

I'm facing an issue, that I cannot make some fields optional because of hardcoded
validation of address fields. Moreover, validation is made in private method, so I can't use plugin for it.

Hopefully, there is a public validation method analogue, available in address class, that can be extended with plugins.

@vrann
Copy link
Contributor

vrann commented Mar 6, 2017

@vovayatsyuk Please check this PR #8519: it fixes the similar issue of certain attributes being required. If it solves the same issue of some fields being required.

Also, can you provide more details on how do you make those fields optional?

@vrann vrann self-assigned this Mar 6, 2017
@vrann vrann added this to the March 2017 milestone Mar 6, 2017
@vrann
Copy link
Contributor

vrann commented Mar 6, 2017

@vovayatsyuk please merge the PR with the latest develop branch

@vovayatsyuk
Copy link
Member Author

vovayatsyuk commented Mar 7, 2017

  1. Merged. While merging, I've noticed that mine PR is already merged. I think that the Improve address save flow to allow to use custom validators #7552 was merged before.
  2. I've made optional fields with address-field-manager. (It uses the same approach as Magento use for customer prefix, suffix, and other configurable address attributes)

@vrann
Copy link
Contributor

vrann commented Mar 8, 2017

@vovayatsyuk Hi, Pull Request now has 0 Files changed after your last commit. Can you please check what happened? You might overwritten your changes by the mainline.

@vovayatsyuk
Copy link
Member Author

It is because my changes where already merged by magento team:

801c121
bd7eecf

I think there is no point to keep this ticket active. Should I close it?

@vrann
Copy link
Contributor

vrann commented Mar 8, 2017

@vovayatsyuk yes, please

@orlangur
Copy link
Contributor

orlangur commented Mar 9, 2017

@vrann I've noticed in a couple of processed pull requests already that some guys are not using Git properly.

Here you can see that cde83ef commit is present in mainline as 801c121. So, somebody did inappropriate rebase which changed the hash of initial commit. With proper Git usage this PR would be marked as Merged without any additional actions.

Cc: @okorshenko, @maghamed, @ishakhsuvarov

@vovayatsyuk
Copy link
Member Author

@orlangur it was me :(

In my previous PR I've made a mistake and decided to create another one. But someone from magento team already applied my changes from the first PR internally. So this PR become empty as soon as my original commit from the first PR was pushed to dev branch and I merged it in here.

p.s. I'll be careful next time..

@orlangur
Copy link
Contributor

Oh :) That's totally unexpected. Sorry guys for disturbing you, I overlooked the probability of force push by PR author.

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

Successfully merging this pull request may close these issues.

5 participants