refactor: new class based factories

[mblarsen]

@poppabear8883 @cbj4074 I started the conversion of the factories in this repo first, but should've done Velkart first. So I'll do that next, then return to this repo.

But must of the implementation is mostly done. Haven't touched the use of the factories yet.

One issue that I ran into was that in some places the repository was used to get the class and in others the model directly. I'm not sure what the "plan" is there.

I'll gladly change my code so do either.

What I've done now is use the HasFactory trait from Laravel 8. It sits on the model and helps you get the factory based on some naming convention.

We could instead add a factory() method to the repos to achieve the same.

It is fine that Song::factory() works. Since Song is an implementation (of something). But the issue with this approach is that in say the factory for Album that uses Song::factory(). You can only get the Song implementation.

Is the aim to decouple Eloquent entirely (looking at you $repo->class())? In that case I'll do something similar by adding a ->factory().

In any case I'll work on Velkart first.

[mblarsen]

See also the changes to Velkart https://github.com/indiehd/velkart/pull/46

Note the use of factories in tests.

It more or less assumes that the model class implements a static factory() method. But at least it is abstracted.

What I was asking above was to internalize this in the repo. So that instead of calling:

$repo->modelClass()::factory()

We serve the factory directly:

$repo->factory()

[mblarsen]

@cbj4074 btw

Album factory has will create songs no matter state 'withSongs' is used or not. Is this intended?

[cbj4074]

@mblarsen Regarding this bit:

One issue that I ran into was that in some places the repository was used to get the class and in others the model directly. I'm not sure what the "plan" is there.

@poppabear8883 and I discussed exactly this issue yesterday on Discord (in the General channel) and came to the same conclusion, in that we should decouple to whatever extent possible.

As you noted, we haven't been entirely consistent as to how we retrieve model class names in tests and factories, but we should definitely fix that (ideally, as part of this effort).

I think it will be difficult to decouple the repositories from Eloquent completely, but I don't see any good reason not to decouple where possible, so I like the idea of adding the dedicated factory() method, as you propose.

And yes, let's serve the factory directly, per your second comment.

[cbj4074]

@mblarsen Regarding your question:

Album factory has will create songs no matter state 'withSongs' is used or not. Is this intended?

You don't miss a thing, do you? :)

Yes, it's intentional, but it should be changed.

You may recall the conversation we had about this several months back:

https://github.com/indiehd/web-api/issues/180

Maybe we should create a new issue to track this problem (feel free to do so if you think it's appropriate).

[mblarsen]

@cbj4074 please have a look at the velkart changes now https://github.com/indiehd/velkart/pull/46 (marked for review)

It uses $repo->factory() now instead in tests and factories.

Once you guys approve the PR I'll update this PR with similar changes.

Btw, is there any reason why model() isn't a part of the repo contract? Lot's of warnings in the tests and code about :)

[cbj4074]

@mblarsen With regard to this question,

Btw, is there any reason why model() isn't a part of the repo contract? Lot's of warnings in the tests and code about :)

yes, because it's defined as an abstract method in the abstract class BaseRepository, here:

https://github.com/indiehd/web-api/blob/8957de5a9f4aa8b7f17d0a4778f4052095117cd8/app/Repositories/BaseRepository.php#L9

Where are you seeing warnings about it while running tests? I've never noticed any.

Nonetheless, if there's a better way to enforce this requirement, I'm all ears. As I recall, there was a compelling reason for which @poppabear8883 and I made that BaseRepository class abstract, rather than relying exclusively on interfaces. I can dig back into that conversation if needed.

[mblarsen]

Here is an example, with class:

(FYI: I added the BaseRepositoryContract in the signature. Same result if using BaseRepository as the type.

The purpose of interfaces is to fix an unbreakable contract. You can sort of do that with abstract classes as well. But inheritance doesn't require that you do not rewrite (override) the parent class methods, thus changing the behaviour. I think PHP behaves rather nicely in that regard and disallows you to change the signature so in that sense it works like an interface.

But if you really want guarantees if a certain behaviour interface is the way to go.

Here is another example with class()

The issue is that all we can say about repo is that is is a BaseRepository and it doesn't have class(), model().

Do you remember what the reasons was not to put class and model in repo interface as well was? (it can be in both places both as abstract and in the interface).

[cbj4074]

Do you remember what the reasons was not to put class and model in repo interface as well was? (it can be in both places both as abstract and in the interface).

Actually, now that I dig deeper, I notice that it is in there:

https://github.com/indiehd/web-api/blob/8957de5a9f4aa8b7f17d0a4778f4052095117cd8/app/Contracts/RepositoryShouldRead.php

As for why the abstract class in RepositoryReadOnlyTestCase, it may have been that we wanted to force protected $repo to be declared (since interfaces can't declare properties), without having to declare it in every test case, per this snippet:

    /**
     * @var BaseRepository
     */
    protected $repo;

    public function setUp(): void
    {
        parent::setUp();

        $this->setRepository();
    }

    /**
     * Sets the $repo property.
     *
     * @return void
     */
    abstract public function setRepository();

[mblarsen]

Actually, now that I dig deeper, I notice that it is in there:

@cbj4074 it is correct that it is in there but the code cannot reason about it until runtime. So when I say my repo type of $repo is BaseRepository or whatever. Then there is no guarantee that my code will run until I actually run it, and not only that. It may not run the +1 time after you ran it.

So if for instance you add RepositoryShouldRead to BaseRepository. Then we get the guarantee and the warnings go away.

And even if you remove the abstract method from BaseRepository you still get the error if omitting the method from AlbumRepository:

From 49f81d60a63fb3696de460feca06d84bd56b1762 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20B=C3=B8cker-Larsen?= <m19n@pm.me>
Date: Wed, 28 Oct 2020 13:18:59 +0800
Subject: [PATCH] feat: let BaseRepository implement RepositoryShouldRead

---
 app/Repositories/BaseRepository.php              | 5 ++---
 tests/Feature/Controllers/ControllerTestCase.php | 3 ++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/Repositories/BaseRepository.php b/app/Repositories/BaseRepository.php
index b0834d22..1d359cb7 100644
--- a/app/Repositories/BaseRepository.php
+++ b/app/Repositories/BaseRepository.php
@@ -2,12 +2,11 @@

 namespace App\Repositories;

+use App\Contracts\RepositoryShouldRead;
 use Illuminate\Pagination\LengthAwarePaginator;

-abstract class BaseRepository
+abstract class BaseRepository implements RepositoryShouldRead
 {
-    abstract public function model();
-
     /**
      * @return mixed
      */
---
2.28.0

[mblarsen]

I committed the above change, but we can jut revert it again if there are issues with it.

[cbj4074]

@mblarsen Those are all good points, and I can't think of any reason not to make the change you did.

[cbj4074]

I just pushed a couple of commits that fix the remaining test failures.

Provided you don't see any issues with those two commits, the only remaining issue is the ignored tests.

I'm content to merge this, and deal with those in a separate PR, if you are.

[codecov[bot]]

Codecov Report

Merging #194 into master will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #194   +/-   ##
=========================================
  Coverage     70.95%   70.95%           
  Complexity      409      409           
=========================================
  Files           113      113           
  Lines          1105     1105           
=========================================
  Hits            784      784           
  Misses          321      321           

Impacted Files	Coverage Δ	Complexity Δ
app/Repositories/BaseRepository.php	81.25% <ø> (ø)	6.00 <0.00> (ø)
app/User.php	100.00% <ø> (ø)	2.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8957de5...918262a. Read the comment docs.