New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable table name #22409

Closed
webmake opened this Issue Dec 13, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@webmake
Contributor

webmake commented Dec 13, 2017

  • Laravel Version: 5.5.22
  • PHP Version: 7.1
  • Database Driver & Version: sqlite

Description:

I suspect that https://github.com/laravel/framework/pull/22222/files
after this change, stopped working tests, due table cache. Because in 5.5.21 working fine

Steps To Reproduce:

In test file:

        $user = factory(User::class)->create();
        $address = factory(Address::class)->create([
            'user_id' => $user->id,
        ]);

        $this->initPHPUnitMock(SomeService::class)
            ->expects($this->once())
            ->method('myMethod')
            ->with($user, $address);
       this->app->make(OtherService::class)->otherMethod($user->id);

in code:

        $user = $this->finderService->getUser($id);
        $address = $user ->address;
        $this->someService->myMethod($user, $address);

Relation in user:

    public function address(): HasOne
    {
        return $this->hasOne(Address::class, 'user_id', 'id')
            ->where('main', true);
    }

Gives an error:

Expectation failed for method name is equal to "myMethod" when invoked 1 time(s)
Parameter 2 for invocation App\Services\SomeService::myMethod(App\User Object (...), App\Address Object (...)): void does not match expected value.
Failed asserting that two objects are equal. <Click to see difference>

Diff:

App\Address Object (
    'casts' => Array (...)
    'fillable' => Array (...)
    'connection' => 'sqlite_testing'
    'table' => 'addresses'
    'primaryKey' => 'id'
    'keyType' => 'int'
    'incrementing' => true
    'with' => Array ()
    'withCount' => Array ()
    'perPage' => 15
    'exists' => true
    'wasRecentlyCreated' => false
    'attributes' => Array (...)
    'original' => Array (...)
    'changes' => Array ()
    'dates' => Array ()
    'dateFormat' => null
    'appends' => Array ()
    'dispatchesEvents' => Array ()
    'observables' => Array ()
    'relations' => Array ()
    'touches' => Array ()
    'timestamps' => true
    'hidden' => Array ()
    'visible' => Array ()
    'guarded' => Array (...)
)
vs
App\Address Object (
    'casts' => Array (...)
    'fillable' => Array (...)
    'connection' => 'sqlite_testing'
    'table' => null
    'primaryKey' => 'id'
    'keyType' => 'int'
    'incrementing' => true
    'with' => Array ()
    'withCount' => Array ()
    'perPage' => 15
    'exists' => true
    'wasRecentlyCreated' => false
    'attributes' => Array (...)
    'original' => Array (...)
    'changes' => Array ()
    'dates' => Array ()
    'dateFormat' => null
    'appends' => Array ()
    'dispatchesEvents' => Array ()
    'observables' => Array ()
    'relations' => Array ()
    'touches' => Array ()
    'timestamps' => true
    'hidden' => Array ()
    'visible' => Array ()
    'guarded' => Array (...)
)

As you can see now it returns null, instead of real table

@webmake

This comment has been minimized.

Show comment
Hide comment
@webmake

webmake Dec 13, 2017

Contributor

Yes, it is. Mentioned commit caused that error.
Seems that, untill this change, table attribute was null, so comparison was null === null. So now $address = factory(Address::class)->create() object is with filled table name (no problem), but problem that fetching via relation does not work in same way as via create method.

I guess hydrate was missed in that commit, because here is creating new instance, without transfering all data from $this (builder) to $instance variable

    public function hydrate(array $items)
    {
        $instance = $this->newModelInstance();

        return $instance->newCollection(array_map(function ($item) use ($instance) {
            return $instance->newFromBuilder($item);
        }, $items));
    }

In other words $this->query->from is addreses, while $instance->table is still null, or even getTable should be called in newFromBuilder method, I don't know

Contributor

webmake commented Dec 13, 2017

Yes, it is. Mentioned commit caused that error.
Seems that, untill this change, table attribute was null, so comparison was null === null. So now $address = factory(Address::class)->create() object is with filled table name (no problem), but problem that fetching via relation does not work in same way as via create method.

I guess hydrate was missed in that commit, because here is creating new instance, without transfering all data from $this (builder) to $instance variable

    public function hydrate(array $items)
    {
        $instance = $this->newModelInstance();

        return $instance->newCollection(array_map(function ($item) use ($instance) {
            return $instance->newFromBuilder($item);
        }, $items));
    }

In other words $this->query->from is addreses, while $instance->table is still null, or even getTable should be called in newFromBuilder method, I don't know

@themsaid

This comment has been minimized.

Show comment
Hide comment
@themsaid

themsaid Dec 18, 2017

Member

I have no clue what the issue is, can you please rephrase what you're saying?

Member

themsaid commented Dec 18, 2017

I have no clue what the issue is, can you please rephrase what you're saying?

@webmake

This comment has been minimized.

Show comment
Hide comment
@webmake

webmake Dec 18, 2017

Contributor

Problem is, that calling $address = $user->address; via relation, that object doesn't have filled table, so objects are inequal

again, full POC:
Example.php

<?php
namespace Tests\Unit;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Foundation\Testing\DatabaseMigrations;
use Tests\CreatesApplication;

/**
 * Class AuthenticateAsTest
 * @package Tests\Unit
 */
class ExampleTest extends \Illuminate\Foundation\Testing\TestCase
{
    use DatabaseMigrations, CreatesApplication;

    /**
     * @throws \InvalidArgumentException
     * @throws \LogicException
     * @throws \PHPUnit\Framework\Exception
     * @throws \PHPUnit\Framework\MockObject\RuntimeException
     * @throws \ReflectionException
     */
    public function test_relations(): void
    {
        /** @var \Illuminate\Database\Eloquent\Factory $factory */
        app(\Illuminate\Database\Eloquent\Factory::class)->define(User::class, function(\Faker\Generator $faker) {
            return [
                'id' => $faker->unique()->randomNumber(5),
            ];
        })->define(Address::class, function(\Faker\Generator $faker) {
            return [
                'id' => $faker->unique()->randomNumber(5),
                'main' => $faker->boolean,
            ];
        });
        $user = factory(User::class)->create();
        $address = factory(Address::class)->create([
            'user_id' => $user->id,
            'main' => true,
        ]);
        $address->wasRecentlyCreated = false;

        $builder = $this->getMockBuilder(SomeService::class)->disableOriginalConstructor();
        $mock = $builder->getMock();

        $this->app->instance(SomeService::class, $mock);

        $mock->expects($this->once())
            ->method('myMethod')
            ->with($address);
        $this->app->make(OtherService::class)->otherMethod($user->id);
    }
}

class SomeService
{
    public function myMethod($address)
    {
    }
}

class OtherService
{
    public function otherMethod(int $id)
    {
        $user = app(User::class)->newQuery()->find($id);
        $address = $user->address;
        app(SomeService::class)->myMethod($address->fresh());
    }
}

class User extends Model
{
    public $timestamps = false;

    public function address(): HasOne
    {
        return $this->hasOne(Address::class, 'user_id', 'id')
            ->where('main', true);
    }
}

class Address extends Model
{
    public $timestamps = false;
}

2017_12_13_000000_create_test_relations.php

<?php
declare(strict_types = 1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;

class CreateTestRelations extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up(): void
    {
        Schema::create('users', function(Blueprint $table) {
            $table->increments('id');
        });
        Schema::create('addresses', function(Blueprint $table) {
            $table->increments('id');
            $table->unsignedInteger('user_id');
            $table->boolean('main');
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down(): void
    {
        // Not necessary
    }
}

Failure diff:
image

Since 5.5.22 that test fails, but if you install 5.5.21 - this test pass with green.
Failure reason is exactly due this

-            return str_replace('\\', '', Str::snake(Str::plural(class_basename($this))));

change into

+	        $this->setTable(str_replace(
 +		        '\\', '', Str::snake(Str::plural(class_basename($this)))
 +	        ));

Even more I modified in my vendor that file back into return instead setTable, same test works as in previous laravel versions. So in my oppinion, that change was incomplete, because freshly loaded object via relation does not fill table field (seen on the right side in picture)

Contributor

webmake commented Dec 18, 2017

Problem is, that calling $address = $user->address; via relation, that object doesn't have filled table, so objects are inequal

again, full POC:
Example.php

<?php
namespace Tests\Unit;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Foundation\Testing\DatabaseMigrations;
use Tests\CreatesApplication;

/**
 * Class AuthenticateAsTest
 * @package Tests\Unit
 */
class ExampleTest extends \Illuminate\Foundation\Testing\TestCase
{
    use DatabaseMigrations, CreatesApplication;

    /**
     * @throws \InvalidArgumentException
     * @throws \LogicException
     * @throws \PHPUnit\Framework\Exception
     * @throws \PHPUnit\Framework\MockObject\RuntimeException
     * @throws \ReflectionException
     */
    public function test_relations(): void
    {
        /** @var \Illuminate\Database\Eloquent\Factory $factory */
        app(\Illuminate\Database\Eloquent\Factory::class)->define(User::class, function(\Faker\Generator $faker) {
            return [
                'id' => $faker->unique()->randomNumber(5),
            ];
        })->define(Address::class, function(\Faker\Generator $faker) {
            return [
                'id' => $faker->unique()->randomNumber(5),
                'main' => $faker->boolean,
            ];
        });
        $user = factory(User::class)->create();
        $address = factory(Address::class)->create([
            'user_id' => $user->id,
            'main' => true,
        ]);
        $address->wasRecentlyCreated = false;

        $builder = $this->getMockBuilder(SomeService::class)->disableOriginalConstructor();
        $mock = $builder->getMock();

        $this->app->instance(SomeService::class, $mock);

        $mock->expects($this->once())
            ->method('myMethod')
            ->with($address);
        $this->app->make(OtherService::class)->otherMethod($user->id);
    }
}

class SomeService
{
    public function myMethod($address)
    {
    }
}

class OtherService
{
    public function otherMethod(int $id)
    {
        $user = app(User::class)->newQuery()->find($id);
        $address = $user->address;
        app(SomeService::class)->myMethod($address->fresh());
    }
}

class User extends Model
{
    public $timestamps = false;

    public function address(): HasOne
    {
        return $this->hasOne(Address::class, 'user_id', 'id')
            ->where('main', true);
    }
}

class Address extends Model
{
    public $timestamps = false;
}

2017_12_13_000000_create_test_relations.php

<?php
declare(strict_types = 1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;

class CreateTestRelations extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up(): void
    {
        Schema::create('users', function(Blueprint $table) {
            $table->increments('id');
        });
        Schema::create('addresses', function(Blueprint $table) {
            $table->increments('id');
            $table->unsignedInteger('user_id');
            $table->boolean('main');
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down(): void
    {
        // Not necessary
    }
}

Failure diff:
image

Since 5.5.22 that test fails, but if you install 5.5.21 - this test pass with green.
Failure reason is exactly due this

-            return str_replace('\\', '', Str::snake(Str::plural(class_basename($this))));

change into

+	        $this->setTable(str_replace(
 +		        '\\', '', Str::snake(Str::plural(class_basename($this)))
 +	        ));

Even more I modified in my vendor that file back into return instead setTable, same test works as in previous laravel versions. So in my oppinion, that change was incomplete, because freshly loaded object via relation does not fill table field (seen on the right side in picture)

@armandsar

This comment has been minimized.

Show comment
Hide comment
@armandsar

armandsar Dec 18, 2017

Contributor

Seeing same issue after upgrade to 5.2.22 from 5.2.21.

Simple way to replicate:

 $user = factory(User::class)->create()->fresh();

 $this->assertEquals($user, $user->fresh());

Will fail with table being null.

Contributor

armandsar commented Dec 18, 2017

Seeing same issue after upgrade to 5.2.22 from 5.2.21.

Simple way to replicate:

 $user = factory(User::class)->create()->fresh();

 $this->assertEquals($user, $user->fresh());

Will fail with table being null.

@webmake

This comment has been minimized.

Show comment
Hide comment
@webmake

webmake Dec 18, 2017

Contributor

Nice @armandsar , your version is more shorter to replicate issue, I tested your variant

Contributor

webmake commented Dec 18, 2017

Nice @armandsar , your version is more shorter to replicate issue, I tested your variant

@themsaid

This comment has been minimized.

Show comment
Hide comment
@themsaid

themsaid Dec 19, 2017

Member

Change reverted

Member

themsaid commented Dec 19, 2017

Change reverted

@themsaid themsaid closed this Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment