rinvex / laravel-categories

Rinvex Categorizable is a polymorphic Laravel package, for category management. You can categorize any eloquent model with ease, and utilize the power of Nested Sets, and the awesomeness of Sluggable, and Translatable models out of the box.
MIT License
454 stars 67 forks source link

morphToMany method in Categorizable trait isn't compatible with Model's morphToMany method #11

Closed pilishen closed 6 years ago

pilishen commented 6 years ago

in Illuminate\Database\Eloquent\Concerns\HasRelationships trait, which used by Illuminate\Database\Eloquent\Model,

public function morphToMany($related, $name, $table = null, $foreignKey = null, $relatedKey = null, $inverse = false)    {...}

there are only six parameters. while in Categorizable trait:

abstract public function morphToMany($related, $name, $table = null, $foreignPivotKey = null,$relatedPivotKey = null, $parentKey = null,$relatedKey = null, $inverse = false);

there are eight parameters, which cause the laravel's complain.

currently i just remove $relatedPivotKey = null, $parentKey = null,, and it works.

or should i put these two in the last?

Omranic commented 6 years ago

This is fixed on the develop branch, exactly at the following command 21f664a6

pilishen commented 6 years ago

well, it's weird. actually i'm using the develop branch with commit c802100 you mean that commit c802100 is already drop support for laravel versions under 5.5 ?

Omranic commented 6 years ago

Currently it's not, but will drop it once L5.5 officially released. What I meant is that reported bug already fixed on your used commit on the develop branch, do you still have any issues?

pilishen commented 6 years ago

yeah, it just happened on commit c802100, which is the latest one

Omranic commented 6 years ago

Well, that's not possible, here's both links for Laravel source code & it's compatible package code: Laravel: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L343 Our Package: https://github.com/rinvex/categorizable/blob/develop/src/Traits/Categorizable.php#L53

Same signature, where's the issue then?

pilishen commented 6 years ago

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L333

as you can see, this is from 5.4 currently master branch of laravel ,is actually 5.5, we may suppose

pilishen commented 6 years ago

or maybe we can say that it's actually drop releases under 5.5? what concerns me most is , you are always chasing the newest versions of laravel, and without backward compatibility. truth is, in production area, lots of projects are still under 5.4. this may result in less usage of your wonderful package. to be honest, i did upgrade laravel from 5.2 to 5.4 all the way up in order to use your package. wonder your thoughts on this criteria?

Omranic commented 6 years ago

Yeah, I see. Well, I agree.

That's why all of our newest packages still untagged and considered beta. That's actually the main reason behind not pushing the packages into stable tagline since there's a lot happening and backward compatibility not guaranteed at this stage. We really need to use both PHP 7.0 & Laravel 5.5 at least as a baseline for all of our packages, and we don't have the resources (and we don't want to actually) support older releases.

What I can assure you is that once L5.5 is officially released we'll work hard to merge all modifications for all packages into master and tag a stable release. You can depend on these stable releases then and backward compatibility will be guaranteed and respected for future releases. Even with the after next major Laravel release 5.6 we're planning to branch out every major version of our packages for security & bug fixes, like v1.x branch for example.

I hope this will make you more comfortable and give some trust for package users, at least starting from the next stable release, which is scheduled as mentioned after official Laravel 5.5 release.

pilishen commented 6 years ago

looking forward and best wishes~ thanks, man~ 👍 👍