edvinaskrucas / notification

Notification package for Laravel
MIT License
526 stars 98 forks source link

Does not work with Laravel 4.2 #29

Closed mstnorris closed 10 years ago

mstnorris commented 10 years ago

I am using essentially the same code from two projects. One of them doesn't work; and after a lot of trial and error as to why it doesn't - (the notifications do not show, and there are no errors) I have to conclude that it is because the latter project which doesn't show the notifications is running Laravel 4.2.

deletosh commented 10 years ago

+1

MisterMike commented 10 years ago

Does not work with Laravel 4.2.6. When Debugging is enabled in the Laravel App, it returns this Exception:

ErrorException (E_UNKNOWN) Declaration of Krucas\Notification\Collection::contains() should be compatible with Illuminate\Support\Collection::contains($value)

I think there's a problem with namespaces. The used call is Notification::showAll()

diZturbio commented 10 years ago

I am upgrading a site I had built in Laravel 4.0 to Laravel 4.2 and I also ran into the same error as MisterMike, and the call I am using is also {{ Notification::showAll() }}. If I remove those calls, everything else in the site seems to be working and properly updated.

stueynet commented 10 years ago

Was fine with 4.2.6 but when I upgrade to 4.2.7 I get the same error.

spajz commented 10 years ago

I get the same error for Laravel 4.2.7 after upgrade.

rahulmkhj commented 10 years ago

I too am facing same issue, did anyone got a fix for this??

spajz commented 10 years ago

@rahulmkhj in message above mikedfunk Fix worked for me https://github.com/mikedfunk/notification/commit/1cff280894791c40114d998502f70a9075bbcf43

duellsy commented 10 years ago

This still looks to be busted for us, the only way to fix is to manually modify the file ourselves which will be lost if there's any new releases that don't include the fix for this

diZturbio commented 10 years ago

@duellsy I had applied this fix manually and it worked for me, meanwhile a better fix got merged into the master branch (https://github.com/edvinaskrucas/notification/pull/33), and now I can simply get a fixed version through composer, just make sure you're requiring version 3.* of this package in your composer.json

duellsy commented 10 years ago

@diZturbio ahh awesome, didn't see the v3.0 release, thanks.

rahulmkhj commented 10 years ago

Changing to 3.* worked for me, thanks!