Skip to content
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

Set null default value to max #191

Merged
merged 1 commit into from
Nov 2, 2017
Merged

Conversation

pabloveintimilla
Copy link
Contributor

This avoid this error:
Type error: Too few arguments to function Avanzu\AdminThemeBundle\Event\NotificationListEvent::__construct(), 0 passed in /var/www/html/cti/vendor/avanzu/admin-theme-bundle/Controller/NavbarController.php on line 39 and exactly 1 expected

This avoid this error:
Type error: Too few arguments to function Avanzu\AdminThemeBundle\Event\NotificationListEvent::__construct(), 0 passed in /var/www/html/cti/vendor/avanzu/admin-theme-bundle/Controller/NavbarController.php on line 39 and exactly 1 expected
Copy link
Collaborator

@shakaran shakaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pabloveintimilla instead set null as default, probably it is more suitable set 0 as integer? Also I am seeing a getTotal() and getMax(), I am not sure if the getMax() is redundant or it never should be major than total (i didn't inspect or check this part of code still)

@pabloveintimilla
Copy link
Contributor Author

@shakaran i understand that the variable "max" is useful to set maximun number of notifications displayed in panel, is correct?. If this is correct y recommend set null that represent without limit

@shakaran shakaran merged commit 212a7e0 into avanzu:master Nov 2, 2017
@shakaran
Copy link
Collaborator

shakaran commented Nov 2, 2017

@pabloveintimilla after check the code, it seems that it is not really used in any place that set the maximum of notification.

I update other events using getMax() that probably have the same issue.
f139524

And added some phpdoc:
d273c83

Thanks for your contribution!

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

Successfully merging this pull request may close these issues.

2 participants