self::creating expects a person entry to already be present

jlsjonas commented 5 years ago

Self-made relation... and this is the result

This is a bug.

Prerequisites

[x] Are you running the latest version?
[x] Are you reporting to the correct repository? (enso is made of many specialized packages: https://github.com/laravel-enso)
[x] Did you check the documentation?
[x] Did you perform a cursory search?

Description

from the 11.12 CHANGELOG:

update user and owner seeders with the ones from laravel-enso/core if they are not customized already (optional)

self::creating in the IsPerson trait seems to do an update; thus expecting an entry to already be there.

If I keep my existing UserSeeder configuration, the following error occurs

Steps to Reproduce

migrate:fresh --seed with a pre-11.12 userseeder (except for modifying owner to group)

Expected behavior

Seeding to happen

Actual behavior

Call to member function update() on null

jlsjonas commented 5 years ago

Note that this will also cause issues if we User::Create(...) from inside the application (as adldap2 does for us if we have a new AD/LDAP user).

aocneanu commented 5 years ago

self::creating in the IsPerson trait seems to do an update; thus expecting an entry to already be there. it is indeed

If I keep my existing UserSeeder configuration, the following error occurs don't use the latest one

Note that this will also cause issues if we User::Create(...) from inside the application (as adldap2 does for us if we have a new AD/LDAP user).

use factory(User::class)->create() instead of User::create, and adapt the factory if you need so

I still don't understand how using email as the relationship key makes sense here but that's another issue 😅

where did you saw that? we have a person_id in the user's table for the relation.

jlsjonas commented 5 years ago

use factory(User::class)->create() instead of User::create, and adapt the factory if you need so

Are we really supposed to use factory() for a user creation? That does not feel like the Laravel way, is it? (outside of a seeder at least).

where did you see that? we have a person_id in the user's table for the relation.

Ye I noticed meanwhile, I saw that in a previous discussion we had. Sorry.

Seeing the actual implementation would mean I could have multiple users ( authenticatables ) attached to 1 person? 👍

jlsjonas commented 5 years ago

use factory(User::class)->create() instead of User::create, and adapt the factory if you need so

Causes the same issue; as the person still doesn't exist. I'm also pretty sure no library will do a factory(User::class)->create([...]) when trying to implement a new user. (and it definitely can't expect a tied-in Person class)

self::creating should just be updated to support provisioning the Person if it doesn't exist yet.

jlsjonas commented 5 years ago

And the factory seems to be having issues even when having the exact same code as the core UserSeeder (apart from an extended User; but it extends EnsoUser so there shouldn't be an issue there...)

jlsjonas commented 5 years ago

was still missing the Person Factory... 😅 Managed to seed using the new code. However the User::create([...]) related issue is still there (I just have a workaround for seeding now)

aocneanu commented 5 years ago

@jlsjonas in your issues you always tend to discuss about ~ 5 subjects. And in the following minutes you sort out 2/3 of them. Please be more considerate with my time. I will close this one now and I will kindly ask you to open another one per each subject. Please specify clearly if it's a bug, a question, a request, a discussion etc.

Thank you

jlsjonas commented 5 years ago

@aocneanu the original bug report still exist; the other comments were just related to the debugging process in trying to fix/avoid them.

the exact original bug description still exist

aocneanu commented 5 years ago

I'm also pretty sure no library will do a factory(User::class)->create([...]) when trying to implement a new user. (and it definitely can't expect a tied-in Person class)

This is exactly the reason why there is a factory. And probably soon all Enso's code regarding to creating models will be moved from Models to Factories.

jlsjonas commented 5 years ago

And probably soon all Enso's code regarding to creating models will be moved from Models to Factories.

From my understanding of Laravel; factories are only made to be used for seeding (usually random) data; not as the main implementation method, no?

aocneanu commented 5 years ago

Oh wow,

Factories are not invented in Laravel.

start here

then think about situations where you need a more complex logic to create an object (like Enso's user) and where would you write it

A in one place (factory) and use it all over the place where you need it (controllers, tests, database seeders, tinker)
B recreating that logic all over the place
C hiding it in magic methods, just so somebody could use a third party package and have self::create

take in consideration the fact that in time the app will grow to be more complex and probably you will have more people looking at the code, and you want readability, manageability and so on.

jlsjonas commented 5 years ago

Fair points... and thanks for pointing that out (I didn't necessarily think it was invented in Laravel though 😉 )

(In my personal opinion, at least) I believe that something as common as a User should support self::create (even if just by falling back to the factory); though I do also understand why you wouldn't too. But that's just an opinion I suppose, I'll see in #2 what you decide on that matter.

laravel-enso / people