binary-cats / laravel-sku

Generate SKUs for Eloquent models
MIT License
66 stars 9 forks source link

Creating model returns zero ID value #6

Closed theimerj closed 3 years ago

theimerj commented 4 years ago

Hello, I came across an issue with storing model that also generates SKU.

During debugging, I found out, that when calling

$model->setAttribute($model->setAttribute($field, new SkuGenerator($model));

SkuGenerator class probably is not cast to string, so I ended up with swapping instances of SkuObserver class in Container with edited creating method, that just casts SkuGenerator to string when setting the attribute and everything works as expected.

    /**
     * Handle the "creating" mmodel.
     *
     * @param  \Illuminate\Database\Eloquent\Model $model
     * @return void
     */
    public function creating(Model $model)
    {
        $field = $model->skuOption('field');

        if ($model->skuOption('generateOnCreate')) {
            $model->setAttribute($field, (string) new SkuGenerator($model));
        }
    }

My question is: Do you have any other way to solve this issue, or is it okay to cast it to string just like that? I would create a pull request, but wanted to ask you in advance.

Thank you for your work on this package, I found it very useful!

andruts commented 3 years ago

I was facing the same issue, your workaround solved it.

cyrillkalita commented 3 years ago

SkuGenerator is already castable to string, which is why your solution works. I wonder, if something else is at play?

cyrillkalita commented 3 years ago

@andruts @theimerj can you give me more of an example? I am not facing this issue with a standard Laravel installation:

theimerj commented 3 years ago

I will try and investigate this issue in more depth and will let you know. I suspect possible interference with other packages may be the case. We will see. I will report back soon.

simioluwatomi commented 3 years ago

Just landed here after discovering this weird behavior in which the SKU model_id is saved as 0. I'll use the suggested workaround.

cyrillkalita commented 3 years ago

@simioluwatomi can you dump your setting, model and composer.json here?

andruts commented 3 years ago

@cyrillkalita it happens when I return a recently created model record.

public function store(Request $request)
    {
        $validator = Validator::make($request->all(), [
            'name' => ['required', 'string', 'max:255'],
            'description' => ['required', 'string'],
        ]);

        if ($validator->fails()) {
            return $validator->errors();
        }

        $product = Product::create($request->all());
        $product->save();

        return $product;
    }

but if I search the record work as expected

public function show(Product $product)
    {
        return $product;
    }

Model

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;
use Spatie\MediaLibrary\HasMedia;
use Spatie\MediaLibrary\InteractsWithMedia;
use Wildside\Userstamps\Userstamps;
use \Rinvex\Categories\Traits\Categorizable;
use BinaryCats\Sku\HasSku;

class Product extends Model implements HasMedia
{

    use Userstamps;
    use InteractsWithMedia;
    use Categorizable;
    use HasSku;

    protected $fillable = [
        'name', 'description', 'stock', 'price', 'active'
    ];
    protected $appends = ['cover', 'author', 'images'];

    public function registerMediaCollections(): void
    {
        $this
            ->addMediaCollection('cover')
            ->singleFile();
    }

    public function getCoverAttribute()
    {
        $this->makeHidden(['media']);
        return $this->getFirstMediaUrl('cover');
    }

    public function getImagesAttribute()
    {
        $this->makeHidden(['media']);
        return $this->getMedia()->map(function ($item, $key) {
            $image['id'] = $item['id'];
            $image['url'] = $item->getUrl();
            return $image;
        });
    }

    public function getAuthorAttribute()
    {
        $this->makeHidden(['creator']);
        return $this->creator;
    }
}

composer.json

{
    "name": "laravel/laravel",
    "type": "project",
    "description": "The Laravel Framework.",
    "keywords": [
        "framework",
        "laravel"
    ],
    "license": "MIT",
    "require": {
        "php": "^7.2.5",
        "binary-cats/laravel-sku": "^0.2.0",
        "doctrine/dbal": "^2.10",
        "fideloper/proxy": "^4.2",
        "fruitcake/laravel-cors": "^1.0",
        "guzzlehttp/guzzle": "^6.3",
        "laravel/framework": "^7.0",
        "laravel/sanctum": "^2.4",
        "laravel/tinker": "^2.0",
        "laravel/ui": "^2.0",
        "rinvex/laravel-categories": "^4.1",
        "spatie/laravel-medialibrary": "^8.3",
        "spatie/laravel-permission": "^3.13",
        "spatie/laravel-translatable": "^4.4",
        "wildside/userstamps": "^2.0"
    },
    "require-dev": {
        "barryvdh/laravel-debugbar": "^3.3",
        "barryvdh/laravel-ide-helper": "^2.7",
        "facade/ignition": "^2.0",
        "fzaninotto/faker": "^1.9.1",
        "mockery/mockery": "^1.3.1",
        "nunomaduro/collision": "^4.1",
        "phpunit/phpunit": "^8.5"
    },
    "config": {
        "optimize-autoloader": true,
        "preferred-install": "dist",
        "sort-packages": true
    },
    "extra": {
        "laravel": {
            "dont-discover": []
        }
    },
    "autoload": {
        "psr-4": {
            "App\\": "app/"
        },
        "classmap": [
            "database/seeds",
            "database/factories"
        ]
    },
    "autoload-dev": {
        "psr-4": {
            "Tests\\": "tests/"
        }
    },
    "minimum-stability": "dev",
    "prefer-stable": true,
    "scripts": {
        "post-autoload-dump": [
            "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump",
            "@php artisan package:discover --ansi"
        ],
        "post-root-package-install": [
            "@php -r \"file_exists('.env') || copy('.env.example', '.env');\""
        ],
        "post-create-project-cmd": [
            "@php artisan key:generate --ansi"
        ]
    }
}
cyrillkalita commented 3 years ago

So here is what I have done:

laravel new products
composer require binary-cats/laravel-sku doctrine/dbal guzzlehttp/guzzle laravel/sanctum laravel/tinker laravel/ui rinvex/laravel-categories spatie/laravel-medialibrary spatie/laravel-permission spatie/laravel-translatable wildside/userstamps

set up database

artisan create_products_table --create

as (cut for brevity)

...
$table->id();
$table->string('sku');
$table->string('name');
$table->string('description');
$table->bigInteger('created_by');
$table->bigInteger('updated_by');
$table->timestamps();
...

then

php artisan vendor:publish --provider="Spatie\MediaLibrary\MediaLibraryServiceProvider" --tag="migrations"
artisan migrate
artisan tinker

then in tinker

# make user
$user = factory(App\User::class)->create();
# authenticate
Auth::login($user);
# create product just as you do that in controller
$product = Product::create([
    'name' => 'product',
    'description' => 'description',
]);
$product->save();

here is the $product:

App\Product {
    name: "product",
    description: "description",
    created_by: 1,
    updated_by: 1,
    sku: BinaryCats\Sku\Concerns\SkuGenerator {#4270},
    updated_at: "2020-08-28 23:30:37",
    created_at: "2020-08-28 23:30:37",
    id: 1,
}

# as array
[
    "name" => "product",
    "description" => "description",
    "created_by" => 1,
    "updated_by" => 1,
    "sku" => "PRO-10867546",
    "updated_at" => "2020-08-28T23:33:01.000000Z",
    "created_at" => "2020-08-28T23:33:01.000000Z",
    "id" => 1,
]

Now, when i run it is controller:

artisan make:controller WelcomeController --i

and tie it to root route in routes\web.php as

Route::get('/', WelcomeController::class);

in WelcomeController::__invoke()

/**
     * Handle the incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return \Illuminate\Http\Response
     */
    public function __invoke(Request $request)
    {
        # make user
        $user = factory(\App\User::class)->create();
        # authenticate
        Auth::login($user);
        # create product just as you do that in controller
        $product = \App\Product::create([
            'name' => 'product',
            'description' => 'description',
        ]);
        # this here does nothing
        $product->save();
        # Return
        return $product;
    }

When you return Eloquent model directly, it is converted to json;

{"name":"product","description":"description","created_by":1,"updated_by":1,"sku":"PRO-37038186","updated_at":"2020-08-29T00:03:14.000000Z","created_at":"2020-08-29T00:03:14.000000Z","id":1,"cover":"","author":{"id":1,"name":"Vernon Leffler","email":"carolina.schroeder@example.com","email_verified_at":"2020-08-29T00:03:14.000000Z","created_at":"2020-08-29T00:03:14.000000Z","updated_at":"2020-08-29T00:03:14.000000Z"},"images":[]}

A few comments here:

  1. Your code works; at least in tinker
  2. There is no need to create() and immediately save(); create will persist (unlike make() or new statement)

I still think there is something else at play here;

cyrillkalita commented 3 years ago
$request->all()

For paranoia sake, i even tried this:

/**
     * Handle the incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return \Illuminate\Http\Response
     */
    public function __invoke(Request $request)
    {
        $request->merge([
            'name' => 'product',
            'description' => 'description',
        ]);

        $validator = \Validator::make($request->all(), [
            'name' => ['required', 'string', 'max:255'],
            'description' => ['required', 'string'],
        ]);

        if ($validator->fails()) {
            return $validator->errors();
        }
        # make user
        $user = factory(\App\User::class)->create();
        # authenticate
        \Auth::login($user);
        # create product just as you do that in controller
        $product = \App\Product::create($request->all());
        # this here does nothing
        $product->save();
        # Return
        return $product;
    }

with this being the response:

{"name":"product","description":"description","created_by":3,"updated_by":3,"sku":"PRO-90397865","updated_at":"2020-08-29T00:08:24.000000Z","created_at":"2020-08-29T00:08:24.000000Z","id":2,"cover":"","author":{"id":3,"name":"Pink Von","email":"rupert05@example.org","email_verified_at":"2020-08-29T00:08:23.000000Z","created_at":"2020-08-29T00:08:24.000000Z","updated_at":"2020-08-29T00:08:24.000000Z"},"images":[]}

But something caught my eye. I am not sure what is model_id you mention?

andruts commented 3 years ago

I thought that @simioluwatomi refers to id of the recently created record.

This is the response I got with the code previously shared

{
    "name": "Test product",
    "description": "A cool description",
    "created_by": 1,
    "updated_by": 1,
    "sku": "TES-74489301",
    "updated_at": "2020-08-29T00:56:52.000000Z",
    "created_at": "2020-08-29T00:56:52.000000Z",
    "id": 0,
    "cover": "",
    "author": {
        "id": 1,
        "first_name": "Postman Test Admin",
        "last_name": "Postman Test Lastname",
        "email": "admin2@test.com",
        "email_verified_at": null,
        "created_at": "2020-08-01T19:18:04.000000Z",
        "updated_at": "2020-08-01T19:18:04.000000Z",
        "role": "administrator"
    },
    "images": []
}

I already removed the save method, following your comments. If you need some other thing from my setup let me know.

cyrillkalita commented 3 years ago

@andruts out of curiosity and just to confirm, product migration's id column is autoincrement, not just bigInteger?

andruts commented 3 years ago

Im using $table->id(), if im not wrong is an alias for bigIncrements

cyrillkalita commented 3 years ago

@andruts can you try to comment HasSku trait in Product model and manually define

class Product extends Model 
{
   // use HasSku;
...
    /**
     * The model's attributes.
     *
     * @var array
     */
    protected $attributes = [
        'sku' => 'TEST-123',
    ];

and see if the id affected on create?

This way sku value would be non-null hard-coded into model, Sku observers will be disabled - and see if the id value still remains 0.

andruts commented 3 years ago

@cyrillkalita done, the Id is returned correctly.

{
    "sku": "TEST-123",
    "name": "Test product",
    "description": "A cool description",
    "created_by": 1,
    "updated_by": 1,
    "updated_at": "2020-08-29T22:03:52.000000Z",
    "created_at": "2020-08-29T22:03:52.000000Z",
    "id": 2,
    "cover": "",
    "author": {
        "id": 1,
        "first_name": "Postman Test Admin",
        "last_name": "Postman Test Lastname",
        "email": "admin2@test.com",
        "email_verified_at": null,
        "created_at": "2020-08-29T16:11:06.000000Z",
        "updated_at": "2020-08-29T16:11:06.000000Z",
        "role": null
    },
    "images": []
}
cyrillkalita commented 3 years ago

@andruts hm; I am at a loss; can you try upgrading to ^0.3, where the the Generator is force casting to string?

andruts commented 3 years ago

After upgrading I'm receiving the id correctly 👍