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

[FEAT] V7 - Multi user audits #421

Merged
merged 5 commits into from
May 10, 2018
Merged

[FEAT] V7 - Multi user audits #421

merged 5 commits into from
May 10, 2018

Conversation

quetzyg
Copy link
Contributor

@quetzyg quetzyg commented May 10, 2018

Description

The code in this pull request is based on #420 but with a few simplifications.

Work done:

  • Audits are now related to Users via MorphTo;
  • UserResolver now checks multiple guards to resolve Users;
  • Updated migration with new user_type column;
  • Updated Audit factory;
  • Updated tests;
  • Other minor cleanups;

Special thanks to Webparking for their initial work!

@crashkonijn & @remkobrenters

@quetzyg quetzyg changed the title [FEAT] Multi user audits [FEAT] V7 - Multi user audits May 10, 2018
@quetzyg quetzyg merged commit 50bcb41 into master May 10, 2018
@quetzyg quetzyg deleted the feat/multi-user branch May 10, 2018 21:51
@crashkonijn
Copy link

Awesome job, thanks!

@quetzyg
Copy link
Contributor Author

quetzyg commented May 11, 2018

@crashkonijn I should be tagging this in the next couple of days. Meanwhile, give it a go, to see if it works as advertised 😄

@crashkonijn
Copy link

@quetzyg I replaced our own changes with your changes this morning and everything works perfectly!

I take it you're going to tag it with version 7, since these changes break backwards compatibility?

Also, we just made a trait which fixes these issues: #310 by copying the fix for Laravel 5.5. Would it be an idea to include it in the package and let people add it to their models when using Laravel 5.4 or earlier?

@quetzyg
Copy link
Contributor Author

quetzyg commented May 11, 2018

Yup, it's gonna be version 7. In regards to #310, wasn't that fixed upstream?

@crashkonijn
Copy link

It's been fixed in Laravel 5.5, yes. But unfortunately some our projects are stuck at 5.3 for example. Without this fix every boolean and date would be marked dirty and changed on every save. On one of our models that meant this package would register 10 changes, even when nothing actually changed.

I can imagine there are more unfortunate people that have to work with legacy Laravel versions.

@quetzyg
Copy link
Contributor Author

quetzyg commented May 11, 2018

I'm not a fan of fixing upstream issues downstream. Have you considered making a pull request to https://github.com/laravel/framework/ ?

But do share the trait, we might consider including it.

@crashkonijn
Copy link

Do they still accept PR's on versions that old?

And I understand your point, but at the same time it does have a huge impact on the performance of this package (read: almost unusable) on older Laravel versions. That's why I suggested to only include it for people to include it if they need to.

Another option could of course also be to link to this gist in the docs: https://gist.github.com/crashkonijn/7d581e55770d2379494067d8b0ce0f6d :)

@quetzyg
Copy link
Contributor Author

quetzyg commented May 11, 2018

Do they still accept PR's on versions that old?

Try it, it wouldn't hurt, I guess :)

I'll have a look at the gist. 👍

@quetzyg
Copy link
Contributor Author

quetzyg commented May 11, 2018

@crashkonijn https://github.com/owen-it/laravel-auditing-doc/pull/62/files#diff-73654fb858684160f86a6b76afb2a6fdR47 😉

@crashkonijn
Copy link

@quetzyg Awesome, thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants