Skip to content

Conversation

@mtancoigne
Copy link
Contributor

As I can't make the tests yet, let's Travis do this...

@mtancoigne
Copy link
Contributor Author

Great... I'll look at this tomorrow...

It seems legit, I did not create a pivotTable value for tests :) @bravo-kernel Happy exceptions :)

@dereuromark
Copy link
Owner

Wow, doesn't that look way cleaner and easier?

Copy link
Owner

Choose a reason for hiding this comment

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

Did you remove the autodetect here? In case no config is provided? Should we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer the exception and less magic,

@mtancoigne
Copy link
Contributor Author

I'copy/paste the discussion with @bravo-kernel wich bring me here:

[22:23:12 CET] <elcms> ok, let sum this up
[22:23:40 CET] <elcms> we keep usersTable, rolesTable, pivotTable
[22:23:51 CET] <bravo_kernel> check
[22:24:17 CET] <elcms> if pivotTable is empty and we are in multi-group, throw an error for misconfiguration
[22:24:55 CET] <elcms> else, do as natural with the given config : test the strings and load the models
[22:25:18 CET] <bravo_kernel> sounds good to me
[22:25:24 CET] <bravo_kernel> i like exceptions for misconfigurations
[22:25:40 CET] <elcms> me too
[22:25:44 CET] <bravo_kernel> cool :)

TL;DR; we get rid of autoconfig and throw an exception if the pivot table is missing, so we don't even need to parse the table name for a plugin in it.

@bravo-kernel
Copy link
Contributor

@mtancoigne could you please try to prevent pasting personal info into public resource?

Final question about this PR; does this work for namespaced plugins as well? E.g. MyCorp\Plugin.TableClass?

Copy link
Contributor

Choose a reason for hiding this comment

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

This message make me think the table does not exist (in the database). Maybe change it to something like below, would make it consistent with the other feedback as well.

Invalid TinyAuthorize MultiRole Setup (no pivot table found in Configure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the paste...

Copy link
Owner

Choose a reason for hiding this comment

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

I agree. But since we have that logic here why do we not use conventions to find it? The error will then come from the orm and will say exaxtly what it said so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dereuromark I guess it's a preference thing. I always appreciate being informed about the root cause as concrete as possible and this would point me straight to Tiny's config but I can live with the ORM exception as well. Your plugin... your call ☺️

Copy link
Owner

Choose a reason for hiding this comment

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

Since its cached performance is not an issue :)
I like convention driven. But maybe later

@dereuromark dereuromark merged commit 5374555 into dereuromark:master Jan 6, 2016
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.

3 participants