Skip to content
New issue

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

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

Already on GitHub? # to your account

[Bug] $entity_model::isColumnNullable() broken for models that extend abstract class that uses CrudTrait #3063

Closed
trip-somers opened this issue Jul 23, 2020 · 3 comments
Assignees
Labels
Minor Bug A bug that happens only in a very niche or specific use case. Possible Bug A bug that was reported but not confirmed yet.

Comments

@trip-somers
Copy link

Bug report

What I did

Upgraded to v4.1 following instructions in documentation.

What I expected to happen

I expected my CrudControllers for my model classes that extend a common abstract model class to continue working as they did for v3.6 and v4.0.

What happened

Use of the select2 field (and similar/related fields) causes a Cannot instantiate abstract class exception. When new self is called in HasRelationshipFields::getConnectionWithExtraTypeMappings(), "self" references the abstract class rather than actual CRUD model.

What I've already tried to fix it

I've bypassed my abstract class to extend Eloquent and use CrudTrait directly. Doing this for all of my crud models would allow the app to function, but it would also add a lot of boilerplate to each model.

The exception traces to the select2 field's use of isColumnNullable(), which was a CrudTrait method in previous versions. Any other field that uses isColumnNullable() is definitely affected by this. (see: https://github.com/Laravel-Backpack/CRUD/blob/4.0.61/src/app/Models/Traits/CrudTrait.php#L54)

That path ultimately leads to HasRelationshipFields::getConnectionWithExtraTypeMappings() which appears to needlessly re-instantiate after having been called on an instance. Basically, shouldn't this line -- https://github.com/Laravel-Backpack/CRUD/blob/4.1.15/src/app/Models/Traits/HasRelationshipFields.php#L25 -- just use $this->getConnectionName()?

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP VERSION:

PHP 7.3.5-1+ubuntu18.04.1+deb.sury.org+1 (cli) (built: May 3 2019 10:00:24) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.5, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.3.5-1+ubuntu18.04.1+deb.sury.org+1, Copyright (c) 1999-2018, by Zend Technologies

LARAVEL VERSION:

v7.21.0@3ccdb116524de408fdc00715b6f06a1031ddace9

BACKPACK VERSION:

4.1.15@6c751de946a9c8511dd32eb7bfa3ca6a568849f5

@trip-somers
Copy link
Author

If this small change is more problematic than it seems, a smaller work-around that minimizes my boilerplate issue would be to just move the use CrudTrait; out of the abstract and into each model.

@tabacitu tabacitu added Minor Bug A bug that happens only in a very niche or specific use case. Possible Bug A bug that was reported but not confirmed yet. labels Jul 23, 2020
@tabacitu
Copy link
Member

tabacitu commented Jul 23, 2020

I'll let @pxpm investigate, he knows this part of the code better than me, but I just wanted to say @trip-somers - excellent bug report, thank you!

@pxpm
Copy link
Contributor

pxpm commented Oct 20, 2020

Hello @trip-somers

Sorry for taking so much time to get into this one.

I'v checked and we only call getConnectionWithExtraTypeMappings with initialized instances so we are good to remove the "re-initialization" in favor or using $this in the context.

I'v just submited the PR #3273 that fixes this issue.

Going to close this in favor of the PR.

Thanks again for the bug report,
Pedro

@pxpm pxpm closed this as completed Oct 20, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Minor Bug A bug that happens only in a very niche or specific use case. Possible Bug A bug that was reported but not confirmed yet.
Projects
None yet
Development

No branches or pull requests

3 participants