Fix AddressController.php #34152

metacreo · 2023-10-03T17:17:06Z

Questions	Answers
Branch?	8.1.x
Description?	Duplicate function FillWith call in AddressController.php. Address form filled two times. First time normal, second time with empty country. If default country is US, form shows needed states to enter. Fix bugs: #35013, #34080, #33991
Type?	bug fix
Category?	FO
BC breaks?	no
Deprecations?	no
How to test?	Indicate how to verify that this change works as expected.
UI Tests	Please run UI tests and paste here the link to the run. Read this page to know why and how to use this tool..
Fixed issue or discussion?	Fixes #{issue number here}, Fixes #{another issue number here}, Fixes https://github.com/PrestaShop/PrestaShop/discussions/ {#35013 #34080 #33991}

Duplicate call function. Address form filled second time. Fix bugs: #34080 and #33991

prestonBot · 2023-10-03T17:17:09Z

Hello @metacreo!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

prestonBot · 2023-10-03T17:17:10Z

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

The category should be one of these: BO, CO, FO, IN, LO, ME, PM, TE or WS (Read explanation)

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

(Note: this is an automated message, but answering it will reach a real human)

mflasquin

Seems good, but can you tell us where the address form is already filled in the code ? thanks

metacreo · 2023-10-04T10:04:42Z

@mflasquin
AddressController.php

   public function postProcess()
    {
        $this->context->smarty->assign('editing', false);
        $id_address = (int) Tools::getValue('id_address');
        // Initialize address if an id exists
        if ($id_address) {
            $this->address_form->loadAddressById($id_address);
        }

        // Fill the form with data
        $this->address_form->fillWith(Tools::getAllValues());

CustomerAddressForm.php

   public function loadAddressById($id_address)
    {
        $context = Context::getContext();

        $this->address = new Address($id_address, $this->language->id);

        if ($this->address->id === null) {
            return Tools::redirect('pagenotfound');
        }

        if (!$context->customer->isLogged() && !$context->customer->isGuest()) {
            return Tools::redirect('/index.php?controller=authentication');
        }

        if ($this->address->id_customer != $context->customer->id) {
            return Tools::redirect('pagenotfound');
        }

        $params = get_object_vars($this->address);
        $params['id_address'] = $this->address->id;

        return $this->fillWith($params);
    }

    public function fillWith(array $params = [])
    {

As mentioned above public function fillWith(array $params = []) called twice. Second call destroys country->id
classes/form/CustomerAddressForm.php

    public function fillWith(array $params = [])
    {
$trace = debug_backtrace();
dump("called by {$trace[1]['class']} :: {$trace[1]['function']}");
        // This form is tricky: fields may change depending on which country is being selected!

result:
Received normal address
^ "called by CustomerAddressFormCore :: loadAddressById"
Received same address but with default country.
^ "called by AddressControllerCore :: postProcess"

Fix:
AddressController.php Line 69
//$this->address_form->fillWith(Tools::getAllValues());
No bugs. Everything works.

mflasquin

LGTM

kpodemski · 2023-10-04T12:44:36Z

Hello @metacreo

It looks legit. What I suggest now:

rebase into 8.1.x branch, so that we can all have this fix before PrestaShop 9 :)
help QA reproduce this issue without using demo.prestashop.com, the origin of the installation shouldn't matter - so kindly provide them with the steps that allow them to reproduce the issue - from what I see the confusion is between adding and editing the address

metacreo · 2023-10-04T13:09:18Z

rebase ok, but I don`t known how to help QA. They cannot reproduce, although I reproduced on 3 production servers and on demo. There no additional steps. Just go to my account and click Update.

Hlavtox · 2023-10-04T13:12:14Z

Hi @metacreo, if it happens only in some browsers, I don't know how JS error is related to removing something on PHP side?

metacreo · 2023-10-04T13:21:33Z

@Hlavtox Hello.
The fact of the matter is that, for example, on server 1 I only have it with Firefox, on server 2 only with Chrome, and on server 3 on both browsers. PHP versions are identical, PS versions identical. I think that this difference is still related to PHP.

Ok. let to try see what shows dump where no bug in form...

Edit:
Tried. Steel double call
^ "called by CustomerAddressFormCore :: loadAddressById"
^ "called by AddressControllerCore :: postProcess"
but Form is ok, because second address country not US, is my local.
Bump... this is related to country detection from browser... But any way.. second call not needed.

matthieu-rolland

thank you for your contribution @metacreo 👍

Would you mind changing the base of the PR (setting the base as 8.1.x instead of develop)

metacreo · 2023-10-04T17:48:10Z

Please wait. After 8.1.1 -> 8.1.2 This fix not works.

Initialize address if an id exists and prevent fill form with default country second time. If this is only form request. Add param id_country for CustomerAddressForm class.

metacreo

Need review. Basically need serious rework of this functions in CustomerAddressForm, AbstractForm and AddressControllerCore.
In order to not touch a lot of code, the only solution for now is this.

Optimisation

metacreo

Small optimization. Checked on 8.1.1 and 8.1.2. My account address edit, add. Checkout process edit, add.

metacreo · 2023-10-06T05:08:10Z

Seems to final of PR ;)

controllers/front/AddressController.php

kpodemski · 2023-10-06T16:17:21Z

controllers/front/AddressController.php

        if ($id_address) {
            $this->address_form->loadAddressById($id_address);
+            // If this is only form request. Adding param id_country for CustomerAddressForm class.


This looks like a hack honestly, I'll have to look deeper since I'm not sure if we should have to do that there

@kpodemski no... this param needs only for correct form fill.

metacreo · 2023-10-07T06:08:09Z

@kpodemski
this original $this->address_form->fillWith(Tools::getAllValues()); not looks as hack for you ?

Hlavtox

@metacreo Please fill the PR form properly and add some description. Can't process any further without description. :-)

Moreover, #34080 is talking about some javascript issues that were not even reproduced. It should be verified and reproduced first.

metacreo · 2023-11-22T19:01:14Z

@Hlavtox Issue reproduced in #33991 See comment 1729389392 Just QA does not have proper qualification to reproduce this bug.
Bug described in details #34080. No need to duplicate description. I fixed everything on my servers a long time ago, and if you don’t want to, don’t accept it... Closing PR...

Hlavtox · 2023-11-22T19:54:25Z

@metacreo I think I managed to reproduce the issue, I will get back to you.

kpodemski · 2023-12-28T13:43:52Z

@Hlavtox, I'm trying to do some cleaning in PRs, did you really manage to reproduce the issue? 🤔

Hlavtox · 2023-12-28T13:51:54Z

@kpodemski Well, somehow - try it #34080 (comment)

metacreo · 2023-12-28T14:33:36Z

Screencast.from.28.12.2023.16.30.36.webm

No need second 0 param in getValue, because (int).

Fix mistake

Hlavtox · 2024-01-10T18:24:30Z

@metacreo Could you please check, if the address form uses geolocation when creating new address?

I think that by moving the fillWith call, this logic is not processed at all.

Tools::isCountryFromBrowserAvailable() &&
            Country::getByIso($countryIsoCode = Tools::getCountryIsoCodeFromHeader(), true)

metacreo · 2024-01-10T18:48:34Z

@Hlavtox

I think that by moving the fillWith call, this logic is not processed at all.

Tested. This logic not depend on $this->address_form->fillWith(Tools::getAllValues()); in AddressController.php

function fillWith in real called 3 times in original...
2 when form is called and 1 when submitted .. so 3 at all
when called, one is extra
we need change postProcces fn only.... because via postProcces form init and via postProcces form submit.

Detection works ok, but you need all lang-pack to be installed for "auto" country...
Detection works only with new customer who not have addr. after add addr... default country getting from first created addr.

and one more comment: this is not "geolocation", this functions use browser headers 🙂

metacreo · 2024-01-10T19:23:33Z

@Hlavtox I use Moz FF.
set accept lang header via about:config for example fr-FR or en-US or ru-RU (presta reads first value of header) or all comma separated
make sure lang pack in BO installed and setting "Set default country from browser language" is ON
presta grab lang from accept lang header first locale

CustomerAddressForm.php

    public function fillWith(array $params = [])
    {
@dump(Tools::isCountryFromBrowserAvailable());
@dump(Tools::getCountryIsoCodeFromHeader());

Try move $this->address_form->fillWith(Tools::getAllValues()); in AddressController.php
Try change browser locale (accept-lang).

@Hlavtox we need exactly move $this->address_form->fillWith(Tools::getAllValues()); to after submit. Because postProcess function called not only if form submitted but on form init too. That`s now it works ps FrontController... even ajax call goes over postProcess, thus called $this->address_form->loadAddressById($id_address) to fill form.
In this case, this bug occurs precisely because the function is called one extra time. When this call is made, the form is filled in with the default country.

I don’t understand why you are going into the weeds, starting to rewrite code that has been working perfectly for many years

metacreo · 2024-01-11T17:42:02Z

@ChillCode hello,
Please continue here #34636

metacreo · 2024-01-11T20:16:49Z

I found out where this whole story came from #27187

Fix AddressController.php

9fd4adf

Duplicate call function. Address form filled second time. Fix bugs: #34080 and #33991

metacreo requested a review from a team as a code owner October 3, 2023 17:17

prestonBot added Bug fix Type: Bug fix BC break Type: Introduces a backwards-incompatible break labels Oct 3, 2023

mflasquin reviewed Oct 4, 2023

View reviewed changes

mflasquin previously approved these changes Oct 4, 2023

View reviewed changes

matthieu-rolland reviewed Oct 4, 2023

View reviewed changes

matthieu-rolland mentioned this pull request Oct 4, 2023

Fix AddressController.php #34167

Closed

metacreo closed this Oct 4, 2023

metacreo reopened this Oct 4, 2023

prestonBot added the 8.1.x Branch label Oct 4, 2023

Small rework AddressController.php

6e84db7

Initialize address if an id exists and prevent fill form with default country second time. If this is only form request. Add param id_country for CustomerAddressForm class.

metacreo dismissed mflasquin’s stale review via 6e84db7 October 5, 2023 17:59

metacreo commented Oct 5, 2023

View reviewed changes

Update AddressController.php

78ab289

Optimisation

metacreo commented Oct 5, 2023

View reviewed changes

Eliminate double call of fillWith

7c2136a

mflasquin previously approved these changes Oct 6, 2023

View reviewed changes

kpodemski reviewed Oct 6, 2023

View reviewed changes

controllers/front/AddressController.php Outdated Show resolved Hide resolved

kpodemski reviewed Oct 6, 2023

View reviewed changes

tleon previously approved these changes Nov 22, 2023

View reviewed changes

ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Nov 22, 2023

Hlavtox removed the Waiting for QA Status: action required, waiting for test feedback label Nov 22, 2023

Hlavtox requested changes Nov 22, 2023

View reviewed changes

ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Nov 22, 2023

prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Nov 22, 2023

Hlavtox mentioned this pull request Jan 10, 2024

Fix Address form not work properly in customer account area #35016

Closed

This was referenced Jan 10, 2024

Fix Address form not work properly in customer account area #35021

Merged

Fix Address form not work properly in customer account area #35020

Closed

Update AddressController.php

a5e8553

No need second 0 param in getValue, because (int).

metacreo dismissed stale reviews from tleon and LouisAUTHIE via a5e8553 January 10, 2024 18:14

Fix AddressController.php

b2a3c4b

Fix mistake

metacreo requested review from kpodemski, Hlavtox, tleon and LouisAUTHIE January 10, 2024 18:18

Merge branch 'PrestaShop:develop' into patch-1

ee962a0

metacreo mentioned this pull request Jan 15, 2024

Address form not work properly in customer account area #34080

Open

2 tasks

metacreo closed this Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AddressController.php #34152

Fix AddressController.php #34152

metacreo commented Oct 3, 2023 •

edited

prestonBot commented Oct 3, 2023

prestonBot commented Oct 3, 2023

mflasquin left a comment

metacreo commented Oct 4, 2023 •

edited

mflasquin left a comment

kpodemski commented Oct 4, 2023

metacreo commented Oct 4, 2023 •

edited

Hlavtox commented Oct 4, 2023

metacreo commented Oct 4, 2023 •

edited

matthieu-rolland left a comment

metacreo commented Oct 4, 2023

metacreo left a comment •

edited

metacreo left a comment •

edited

metacreo commented Oct 6, 2023

kpodemski Oct 6, 2023

metacreo Oct 7, 2023

metacreo commented Oct 7, 2023

Hlavtox left a comment •

edited

metacreo commented Nov 22, 2023

Hlavtox commented Nov 22, 2023

kpodemski commented Dec 28, 2023

Hlavtox commented Dec 28, 2023

metacreo commented Dec 28, 2023

Hlavtox commented Jan 10, 2024

metacreo commented Jan 10, 2024 •

edited

metacreo commented Jan 10, 2024 •

edited

metacreo commented Jan 11, 2024

metacreo commented Jan 11, 2024

Fix AddressController.php #34152

Fix AddressController.php #34152

Conversation

metacreo commented Oct 3, 2023 • edited

prestonBot commented Oct 3, 2023

prestonBot commented Oct 3, 2023

mflasquin left a comment

Choose a reason for hiding this comment

metacreo commented Oct 4, 2023 • edited

mflasquin left a comment

Choose a reason for hiding this comment

kpodemski commented Oct 4, 2023

metacreo commented Oct 4, 2023 • edited

Hlavtox commented Oct 4, 2023

metacreo commented Oct 4, 2023 • edited

matthieu-rolland left a comment

Choose a reason for hiding this comment

metacreo commented Oct 4, 2023

metacreo left a comment • edited

Choose a reason for hiding this comment

metacreo left a comment • edited

Choose a reason for hiding this comment

metacreo commented Oct 6, 2023

kpodemski Oct 6, 2023

Choose a reason for hiding this comment

metacreo Oct 7, 2023

Choose a reason for hiding this comment

metacreo commented Oct 7, 2023

Hlavtox left a comment • edited

Choose a reason for hiding this comment

metacreo commented Nov 22, 2023

Hlavtox commented Nov 22, 2023

kpodemski commented Dec 28, 2023

Hlavtox commented Dec 28, 2023

metacreo commented Dec 28, 2023

Hlavtox commented Jan 10, 2024

metacreo commented Jan 10, 2024 • edited

metacreo commented Jan 10, 2024 • edited

metacreo commented Jan 11, 2024

metacreo commented Jan 11, 2024

metacreo commented Oct 3, 2023 •

edited

metacreo commented Oct 4, 2023 •

edited

metacreo commented Oct 4, 2023 •

edited

metacreo commented Oct 4, 2023 •

edited

metacreo left a comment •

edited

metacreo left a comment •

edited

Hlavtox left a comment •

edited

metacreo commented Jan 10, 2024 •

edited

metacreo commented Jan 10, 2024 •

edited