Unresolved issues in merged PR34

[HermanPeeren]

In PR #34 there were some unresolved issues. I had added a general remark, with some background explanation, which you might have seen, but I also added a more detailed review with concrete suggestions how to solve it.

The PR was merged however before the review was submitted; so that was still pending. And you might not have seen it. I just realised that and submitted the review I made last Friday. So now the review is visible.

Can you please resolve the issues I indicated in my comment and review? Thank you!

[Rahulsingh1939]

Thanks a lot for pointing this out and for your detailed review, .
I was still learning about these concepts, and actually I was writing tests for the component — so I thought merging first and then solving these issues would be the better approach.
Really appreciate your guidance .

[genr8r]

Thank you both @HermanPeeren and @Rahulsingh1939 for coordinating on this. I will still have a look at both the new items for the next PR and the feedback on the previous one.

[genr8r]

@HermanPeeren this is a huge PR from @Rahulsingh1939 do mind having a look to confirm that he addressed your concerns well so I can close this issue? Or feel free to comment (and close) as appropriate.

[HermanPeeren]

I didn't have time to look at it earlier, but a new mistake was introduced in the corrections of the issues I addressed: Controllers in Joomla have an MvcFactory: the (protected) $factory property. So has the ImportController.

The mistake that was introduced now was based on a wrong advise I gave: I said the MvcFactory of the controller (to inject into the ImportModel) could be retrieved with $this->getMVCFactory(), but that should be $this->factory. Sorry, my mistake. But from there Rahul's correction got a bit off track: as the getMVCFactory() method was missing, the MVCFactoryAwareTrait was added (current line 17). And because that (superfluous) MVCFactoryAwareTrait uses another $mvcFactory property that was not set, 12 lines (lines 102-113) were added to add the MvcFactory again to that controller.

The correct solution is much simpler: in current line 101 just set $model->setMVCFactory($this->factory); to inject the MvcFactory that is in the controller. Then that whole try-catch block plus the assignment to $mvcFactory can be left out (current lines 99-100 and 102-113), as well as the trait in current line 17.

I also have issues with the now generated tests, but will address that coming Monday or Tuesday (cannot do it earlier); it is not related to this issue, so will create a seperate issue for it. Maybe you can split unrelated PRs in seperate PRs in the future (but that only works if they are from seperate branches, I think).

[HermanPeeren]

Small remark:

I like it very much that you call the MvcFactory of com_categories $categoriesMvcFactory, and idem with com_tags, com_menus and com_fields: that makes it clear it is not the MvcFactory of this ProcessorModel. But for consistency I would expect the same for com_content in lines 470, 732 1298 in the ProcessorModel.

[genr8r]

@Rahulsingh1939 please review this issue and let us know if you have any questions or concerns addressing it.

[HermanPeeren]

Closed as last remarks were resolved in pr #46 .