Prestashop: BC on SearchAndResetType forms

[mickaelandrieu]

See 1.7.5.1 version and develop version.

I've noticed while updating training contents, this should be fixed.

Proposal:

1. Revert content of this update
2. Create a new FormType (SearchType.php) with the new features
3. Use the new form type everywhere we need it.
4. Javascript extension(s?) also needs to be updated?

Ping @sarjon / @matks

[matks]

@eternoendless can we add this to 1.7.6 backlog ?

[sarjon]

@mickaelandrieu what BC break are you talking about? :thinking: Can you explain it more?

I can see that SearchAndResetType code has changed, but I cannot see where the BC break was introduced. Is there something that was not working for you? Do you have code example that is not working?

[PierreRambaud]

@sarjon he's talking about https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Form/Admin/Type/SearchAndResetType.php#L49 which is now a mandatory construct parameter.

[sarjon]

Ohhh, then I don't think it's a big deal. It is very very uncommon to create instances of form types as they are configured using Fully Qualified names (for example $this->createForm(SearchAndResetType::class).

@matks wdyt?

[mickaelandrieu]

it's not only that @sarjon (but you're right @PierreRambaud), I think you should try your training module on 1.7.6 ;)

[matks]

I'm afraid @mickaelandrieu is right, we cannot modify such as useful and reusable class with BC

[sarjon]

I think you should try your training module on 1.7.6

Regarding SearchAndResetType, seems to be working. Am I missing something? :thinking:

we cannot modify such as useful and reusable class with BC

You mean add constructor parameter to form type?

[matks]

I think you should try your training module on 1.7.6

Regarding SearchAndResetType, seems to be working. Am I missing something? 🤔

ping @mickaelandrieu

we cannot modify such as useful and reusable class with BC

You mean add constructor parameter to form type?

Yes

[sarjon]

You mean add constructor parameter to form type?

Yes

I know that in theory it's a BC break, but this is a form type, you never create new instances by yourself. This kind of BC break is acceptable imo and will affect very little _if any_ developers.

I think it's better than introducing yet another similar form type which would add more confusion.

[mickaelandrieu]

This kind of BC break is acceptable imo and will affect very little if any developers.

this affects me and probably you (your training module).

But above all, we could easily avoid this BC break by introducing back the setter injection and making it deprecated and removed for the next major: what the hurry now to break stuff for no functional reason?

[khouloudbelguith]

Ping @sarjon, @matks any news?

[mickaelandrieu]

@khouloudbelguith nope and it's shipped now in 1.7.6 so it's too late => closed.

[eternoendless]

we could easily avoid this BC break by introducing back the setter injection and making it deprecated and removed for the next major

What setter? This can still be done in 1.7.6.1 if necessary.

[mickaelandrieu]

What setter? This can still be done in 1.7.6.1 if necessary.

This won't help the module developer to maintain its modules across PrestaShop versions

Now

• before 1.7.6.0
• after 1.7.6.0

If we patch in 1.7.6.1

• before 1.7.6.0
• 1.7.6.0
• 1.7.6.1+ (same code as "before 1.7.6.0 but with a deprecation)

As they'll have to support 1.7.6.0 anyway, this changes nothing for them.

[eternoendless]

It would be considered a bug fix, and people upgrade more easily between patch versions than between minor versions.