IBX-3334: Skipped migrating binary file or its metadata when using the same handler

[barw4]

Question	Answer
JIRA issue	IBX-3334
Type	bug
Target Ibexa version	v3.3
BC breaks	no

As stated in the title, seems like a logical thing to do as that leaves less space for errors.

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/engineering-team).

[alongosz]

I don't follow, how does this solve the issue? Why are you duplicating the code that differs only by 2 lines? Why are there no tests covering the issue?
CS is failing here.

[barw4]

I don't follow, how does this solve the issue?

It solves the issue by not migrating files or metadata to the exact same handler which is prone to errors (like writing the same file to the exact same location), besides, what's the point besides lowering the performance of the command?

Why are you duplicating the code that differs only by 2 lines?

Where exactly is this duplication?

Why are there no tests covering the issue?

FileMigrator and MigrateFilesCommand are not tested at all like all/most commands in the kernel.

CS is failing here.

It's not related. It's failing for some time already for CS in 8.0 in some packages. You can see that this is completely unrelated when you go to the Files tab.

[alongosz]

I don't follow, how does this solve the issue?

It solves the issue by not migrating files or metadata to the exact same handler which is prone to errors (like writing the same file to the exact same location), besides, what's the point besides lowering the performance of the command?

Why are you duplicating the code that differs only by 2 lines?

Where exactly is this duplication?

It seems that both methods accept different types of objects for get_class(...) === get_class(...) lines and additionally one method has 2 extra lines

$metadataCreateStruct->mtime = $binaryFile->mtime;
$metadataCreateStruct->mimeType = $this->fromMetadataHandler->getMimeType($binaryFile->id);

I don't get why we use the same struct for both and why remaining same struct parameters are filled in the only one case.

That said, this could do maybe, given those seem to be different operations with the same naming, but... how does it work that fromMetadataHandler, toMetadataHandler, and fromBinarydataHandler, toBinarydataHandler instances change? This is a service, so its members should represent the same instances across the container. I just feel there's bigger code smell here which should be fixed.

Why are there no tests covering the issue?

FileMigrator and MigrateFilesCommand are not tested at all like all commands in the kernel.

FileMigrator is not a command and the question what exactly about this class. On a side note, it's a big technical debt that we don't test commands. Do you think that if something wasn't done before, we shouldn't improve? :) That said obviously right now I'm concerned about FileMigrator, but maybe let's dig deeper into what I've mentioned in previous paragraph before I can decide what should be covered.

CS is failing here.

Ah true, I'll look into this. In that case it should be resolved separately. I encourage being pro-active in the future :)

[barw4]

I don't get why we use the same struct for both and why remaining same struct parameters are filled in the only one case.

Because this is what is required respectively by IOBinarydataHandler and IOMetadataHandler. Different properties are required for both.

That said, this could do maybe, given those seem to be different operations with the same naming, but... how does it work that fromMetadataHandler, toMetadataHandler, and fromBinarydataHandler, toBinarydataHandler instances change? This is a service, so its members should represent the same instances across the container. I just feel there's bigger code smell here which should be fixed.

Handlers are being set in the ibexa:io:migrate-files command depending on the chosen options here: https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Bundle/EzPublishIOBundle/Migration/MigrationHandler.php#L48.
I haven't changed the way this command works besides slight refactoring and providing a check to avoid unnecessary processing. If you feel like we should rewrite the command completely that is also an option but that shouldn't be the scope of this PR as this is rather to fix the bug blocking the migration.

Do you think that if something wasn't done before, we shouldn't improve?

Sure, I completely agree, that's a great idea, however, I feel like this shouldn't be the scope of this PR as all the commands require such treatment.

[alongosz]

@barw4 you can rebase the changes for CI not to fail

After deeper investigation I see that we have a flaw in the whole design of this command, which contradicts DDD approach with Services. A service should never allow to change its state during execution. If there's some dependency that needs to be set, it should be set within the scope of the method call. Alternatively a command should utilize some form of Factory to build an instance used only for a single runtime.

Given the flaw pre-existed, I can agree that refactoring of this architecture might require more work than we currently have a capacity to do, especially taking into the account BC on SPI.

Do you think that if something wasn't done before, we shouldn't improve?

Sure, I completely agree, that's a great idea, however, I feel like this shouldn't be the scope of this PR as all the commands require such treatment.

This argument never worked neither with me nor the other Maintainers of kernel & core package. It's not about catching up with technical debt of all the commands. It's about covering with unit test a use case that proved as a pain point for a Customer so we avoid further regressions (it's a part of a bug fix).

Moreover I didn't ask for coverage for the commands. I asked for the coverage of current behavior of FileMigrator. Forget about the Command. Have a test that instantiates FileMigrator with proper mocks which reflects the use case of the same and different from->to handlers. It's not a very complicated task. With that covered we can move on.

[barw4]

@alongosz FileMigrator test case has been added and the branch has been rebased.

[sonarqubecloud]

Please retry analysis of this Pull-Request directly on SonarCloud.

[alongosz]

Review ping @adamwojs

[adamwojs]

After deeper investigation I see that we have a flaw in the whole design of this command, which contradicts DDD approach with Services. A service should never allow to change its state during execution. If there's some dependency that needs to be set, it should be set within the scope of the method call. Alternatively a command should utilize some form of Factory to build an instance used only for a single runtime.

The second proposed solution is 100% valid here and it should be pretty easy to introduce. My recommendation would be directly inject \eZ\Bundle\EzPublishIOBundle\Migration\FileMigratorInterface factory into command, as we need to create FileMigrator base on command args/options.

I agree that this could be improved in separate PR.

[adamwojs]

Could you please merge up changes @barw4 ?