Check overrides

[ludoviccardinale]

Questions	Answers
Branch	8.2.x
Description	This Pull Request introduces new functions to the PrestaShop core that aim to resolve a critical issue related to module overrides. The problem occurs when a module is installed, and it attempts to override a class or method that is already overridden by another module. This can lead to conflicts, unexpected behavior, and errors during the installation process.
Type	bug fix
Category	CO
BC breaks	no
Deprecations	no
How to test	To ensure the new functionality works as intended, follow the steps below to test the detection of conflicting overrides during module installation: Prerequisites: PrestaShop Installation: Ensure you have a working installation of PrestaShop. Module A: Prepare a module (ModuleA) that contains an override for a specific class or method. Module B: Prepare another module (ModuleB) that contains an override for the same class or method as ModuleA.
UI Tests
Fixed issue or discussion	https://github.com/PrestaShop/PrestaShop/issues/36617
Related PRs
Sponsor company	Prestashop SA

[ps-jarvis]

Hello @ludoviccardinale!

This is your first pull request on PrestaShop repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

[kpodemski]

@Hlavtox if it compare override functions, and if there is one that already has been overridden then it fails, I think it makes sense to block the installation, you can then ask support to merge overrides and install the module without side effects

[kpodemski]

@ShaiMagal

It seems that you didn't understand this change.

Now situation (before apply PR): Module can't be enabled and you see error message, and eshop owner know, where the problem is. And eshop owner can contact developer.

No, the current situation is that even if there are errors with overrides, the module is silently installed. With reinstallation of the module, the original override (from another module) may be removed mistakenly.

Situation after apply this PR: Module can't be installed and you don't see reason, only fail... So.... No one know there is problem with override... - it will waste developer time for investigating.

No. You see the reason, there will be a message about conflicting override:

$this->_errors[] = Context::getContext()->getTranslator()->trans(
                        'The override file %1$s conflicts with an existing override in %2$s.',
                        [$module_override_path, $prestashop_override_path],
                        'Admin.Modules.Notification'
                    );

@ludoviccardinale PHPCsfixer is unhappy with some of the styling of the code. Could you fix it?

[kpodemski]

@Hlavtox

It's not a bc break. With the situation you described:

Does this mean that two modules with the same override cannot be installed? That is a BC break, because if I am not wrong, a professional user or a module support was able to install this, after manually merging the override.

You have to disable overrides from one module anyway, meaning:

• you try to install a module and it fails
• the module is silently installed
• you contact support because it does not work
• you merge overrides
• you change /modules/conflicting_module/override to /modules/conflicting_module/override-off
• you install it again

Now, the process would be:

• you try to install a module and it fails with an error
• you contact the support
• and all the steps above

So, eventually, it will help with a module being silently installed even if it should not be installed.

[matthieu-rolland]

I'll be in charge of this PR from now on @kpodemski

Missing types and return types.

Perhaps some code like getClassMethodsFromContent should also be moved into dedicated files so it can be unit-tested

yes indeed, I'll do it 👍

[matks]

Hi, I have changed the target branch to 8.2.x because of https://github.com/PrestaShop/PrestaShop/issues/36758

[ShaiMagal]

@Hlavtox WDYT?

[ShaiMagal]

@kpodemski I tested this PR again, and i still don't see any benefit from this.

State: I installed module "A" with override Product.php. And trying to work with module "B" with same override.

without PR: A) I can't install module "B" with same override B) If I uninstall module "A", override is deleted

with PR: A) I can't install module "B" with same override B) If I uninstall module "A", override is deleted

Same behaviour. Only other text message...

What is difference from actual state? What is benefit? What am I missing?

Then I will remove my lock of course. I like moving PrestaShop forward, but I really don't see any benefit. Please, someone explain for me.

[kpodemski]

@ShaiMagal

Have you checked the code? For this scenario:

without PR: A) I can't install module "B" with same override B) If I uninstall module "A", override is deleted

after installing module B with the same override, check the database and see that the module has been added to the PREFIX_module table

with the PR, that shouldn't be the case, and this difference is important

installing overrides happens when you here:

if (!$this->enable()) {
            return false;
        }

so, as you can see in the code, this won't be reached if there is a problem with overrides

[matthieu-rolland]

(pr not ready)

[ShaiMagal]

@kpodemski I still don't understand, what is benefit of this change...

Are we sure, this change is really suited for PS 8.2? Not develop branch?

Are we 100% sure, there will be no impact? I prefer to add this to develop, what do you thing? For develop I don't have problem with this change.

Anyway, function getClassMethodsFromContent is strange for readability and understanding, at least, for me. It must be detaily tested, if this i for PS 8.2... I prefer develop branch.

This behaviour is same for years, I don't think, it's necessary to "fix" it for PS 8.2, there can be any negative impact of this change.

Can we change branch to develop?

What do you think? :)

Then we can approve it without problems. But we need do small changes in code at least. I change my review.

[kpodemski]

@ShaiMagal I think I already explained the change in behavior and why it is a right thing to improve installation process. Installation fails and you still end up having a module... installed :)

Let's wait for @matthieu-rolland to continue with this PR.

[matthieu-rolland]

I just pushed a refacto, and added comments on getClassMethodsFromContent, whose code is not obvious.

I also had to modify getClassMethodsFromContent because I had edge cases were conflicts were not detected (when there's spaces before function name for example)

I'll add tests soon and fix integration tests, still wip

[matthieu-rolland]

Ready for review

[ps-jarvis]

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

• Admin.Modules.Notification
  • [ ] The override file %1$s conflicts with an existing override in %2$s.

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