New configuration keys #18

mtancoigne · 2016-01-05T10:37:49Z

Added some config keys to allow a simpler configuration through loadComponent and

Changed the behaviour of undefined pivot tables(role plugin name, pivot table plugin name).
Changed the behavior for undefined pivot tables : user_roles instead of roles_users (seems more logical to me, as users have roles)
Updated the docs
removed the CLASS_USER constant : custom plugin is handled with the pivotTablePlugin option.

I can't run the tests as i'm not at home, but as far as I tried, it works.

…omponent and changed the behaviour of undefined pivot tables

bravo-kernel · 2016-01-05T10:45:10Z

Nice starting point.

I personally do not like making non-CakePHP defaults the "defaults" for this plugin. For example; the original roles_users pivot table name follows Cake naming standards, changing it to user_roles does not.

dereuromark · 2016-01-05T10:45:46Z

I agree with you in general on the user_roles (use that myself), but the former was and is convention. So I am with @bravo-kernel .

mtancoigne · 2016-01-05T10:46:09Z

ok, and the build is not passing, so... :)

dereuromark · 2016-01-05T10:46:49Z

src/Auth/TinyAuthorize.php

It used to be Users, sure this needs to be singular now?

Not sure but I recall discussing this with you before (and reverting the exact same thing). Might be totally unrelated though 😜

Yeah, I was stuck with an habit of naming my pivot tables in a singular_plural form... I was sure to read that in cake doc some years ago, but it appears i'm totally wrong :) I'm reverting this now.

singular_plural is fine in general, just not for HABTM. And this pivot table is HABTM.
People can use their own convention as custom config.

dereuromark · 2016-01-05T10:52:45Z

Yes, the builds break because you changed config but not the fixtures
https://github.com/el-cms/cakephp-tinyauth/blob/master/tests/TestCase/Auth/TinyAuthorizeTest.php#L23
Just revert the default back to roles_user and it should all be green again.

bravo-kernel · 2016-01-05T10:55:43Z

roles_users

dereuromark · 2016-01-05T11:01:30Z

Yes :P

…d added missing spaces... to correct previous failing PR

dereuromark · 2016-01-05T11:19:24Z

docs/README.md

mtancoigne · 2016-01-05T11:19:43Z

Thanks for your remarks :)

dereuromark · 2016-01-05T11:19:52Z

src/Auth/TinyAuthorize.php

no spacing after cases

damn autoformat...

mtancoigne · 2016-01-05T11:27:12Z

... If there is anything else, i'll do one commit to fix the remaining things instead of small multiple ones...

dereuromark · 2016-01-05T12:05:15Z

src/Auth/TinyAuthorize.php

please look into your changes again and only change whats necessary, those lines are neither correct nor necessary to change.

Sorry for that, I use an autoformat feature on my IDE to keep my code clean between all of my projects... But as it depends from one project to another, I should have checked the spaces changes before commiting.

bravo-kernel · 2016-01-05T12:07:04Z

And more importantly... hang in there buddy, almost there :wink

dereuromark · 2016-01-05T12:07:17Z

@bravo-kernel I am OK with the changes. What do you think?

@mtancoigne Can you please squash all commits?

bravo-kernel · 2016-01-05T12:29:50Z

I think we should make the doc about loading the plugin more explanatory (and consistent with other plugin readmes). Copy-paste from this example if you like https://gist.github.com/bravo-kernel/7219d5bd8295b265b893.

Other than that I just dropped it in my app and no issues there so looks good to me.

dereuromark · 2016-01-05T12:33:46Z

@bravo-kernel That can be added after the PR mege.

bravo-kernel · 2016-01-05T12:38:43Z

True, ship it

dereuromark · 2016-01-05T12:40:51Z

README.md

Can we please keep the loadAll() part?

Is there a need for that now that bootstrap is no longer there?

Good point ;) Derp
Never mind then.

bravo-kernel · 2016-01-05T13:05:39Z

Thanks for including the plugin load Mr. @mtancoigne

New configuration keys

dereuromark · 2016-01-05T13:07:08Z

Thank you! I will soon tag a new 1.2.0 release.

bravo-kernel · 2016-01-05T13:07:23Z

👏

dereuromark · 2016-01-05T14:24:48Z

I am still a little bit hesitant regarding the new plugin keys 'pivotTablePlugin' and 'rolesTablePlugin'.
And what about plugin for Users table?
IMO those should all 3 work as the rest of CakePHP using the prefix dot syntax:

// App
'usersTable' => 'Users',

// User plugin
'usersTable' => 'User.Users',

Those two keys are not necessary then. And Users table can also be pluginized if desired.

mtancoigne · 2016-01-05T18:25:56Z

For usersTable, there seems to be no need, as i did not see any probe to it in the code. The only place where it's used is on lines 368/383 (sorry, I don't know how to reference those in this comment), to create the pivot table name... So if a pivotTablePlugin is specified it will precede the UsersRoles class name. If we use an userTablePlugin, that would make a strange PivotPlugin.UserPlugin.UsersRoles table. As the user object is passed to validate() any fetch on the users table seems useless...

But I may be wrong.

dereuromark · 2016-01-05T18:30:25Z

Why PivotPlugin.UserPlugin.UsersRoles? You can only have one plugin per class name.
And the pivot table must in in one of those (or a totally different one).

mtancoigne · 2016-01-05T20:30:20Z

Just to be sure, creating a usersTablePlugin, even if it's unused, may be useless as there is no naming convention for that kind of thing. A line in the doc to explain that users should NOT include the plugin name in the usersTableName may be enough... If cake had some standards about naming configuration options in those particular cases, it would be obvious to add it, but it's not the case...

I want the advice of @bravo-kernel to push me in :)

(Or someone else too)

bravo-kernel · 2016-01-05T20:48:21Z

Re-reading the comments and looking at the code I believe must side with @dereuromark here if I understand correctly that the starting point of the logic is that there always can/should be:

only one single roles table
only one single pivot table.

If this is indeed the case then the new rolesTablePlugin and pivotTablePlugin do not make much sense anymore, are indeed redundant IMO and should thus be removed.

Where I am lost is how ~~the~~ a plugin is even related to the two tables. As far as I know there (unfortunately) is no way to prefix/distinguish plugin tables so.... what are we even trying to support here in the first case? In all honesty.... I might be totally wrong here so some feedback from @dereuromark would be welcome.

mtancoigne · 2016-01-05T21:08:09Z

As far as I understand the code, there is, for now three config key/values, used in different setups (all for multi-group support):

usersTable to define the users model. It's used when the Users model us not... Users or when the pivotTable is not defined.
rolesTable to define the roles model. It's used when the Roles model us not... well... Roles or when the pivotTable is not defined.
pivotTable is the pivot model.

Then, as the userTable, rolesTable and pivotTable may have been defined in a custom plugin, we need a userTablePlugin, rolesTablePlugin and pivotTablePlugin, to fetch the right models in the right place.

In the code, we have this :

// line 365
// multi-role: reverse engineer name of the pivot table IF NOT DEFINED.
        $rolesTableName = $this->_config['rolesTable'];
        $pivotTableName = $this->_config['pivotTable'];
        $usersTableName = $this->_config['usersTable'];
        if (!$pivotTableName) {
            $tables = [
                $usersTableName,
                $rolesTableName
            ];
            asort($tables);
            $pivotTableName = implode('', $tables);
        }

        // fetch roles directly from the pivot table
        $pivotTablePlugin = $this->_config['pivotTablePlugin'];
        if (!$pivotTablePlugin) {
            $pivotTableName = $pivotTablePlugin . '.' . $pivotTableName;
        }
        $pivotTable = TableRegistry::get($pivotTableName);

So : If the pivot table is not defined it is created from users and role tables. Then the pivotTablePlugin is added, if defined. (note: It should be clear to users in docs, a point on this may be good)

roleTablePlugin and rolesTablePlugin are used to get the available roles in db (_getAvailableRoles())

But usersTable is only used to create a pivot model name if none is given. @dereuromark's idea was to keep a userTablePlugin config key to not disappoint users, but i don't find this pertinent as there are nothing like a standard or convention in cakephp that would make users less disappointed with a key like this. The doc should be enough...

bravo-kernel · 2016-01-05T21:11:25Z

Hmmm, I guess I was a bit too quick with the last point there Mr @mtancoigne . Seems we are not talking about the table but about figuring out which Table class to load in this line and this line correct?

In that light I totally agree with what @dereuromark suggested in #18 (comment) so all we seem to need is just a way to filter/parse the value of the three "old" keys userTable, rolesTable and pivotTable to support CakePHP dotted plugin notation.

Jump on to irc if you like to discuss more efficiently. Good to see you getting involved 👍

bravo-kernel · 2016-01-05T21:14:06Z

ps: check http://book.cakephp.org/3.0/en/appendices/glossary.html and find plugin syntax

dereuromark · 2016-01-05T23:38:31Z

Yes, plugin syntax should be accepted, so people using it won't trip over it, making the additional config keys unncessary :)

mtancoigne added 5 commits January 5, 2016 11:12

Added some config keys to allow a simpler configuration through loadC…

944d21b

…omponent and changed the behaviour of undefined pivot tables

Updated docs

5825ce3

removed useless CLASS_USER constant

525da8b

Updated config keys

4261bfd

Updated tests

573579b

dereuromark reviewed Jan 5, 2016
View reviewed changes

Removed singular name for first pivot table name, split some lines an…

8e6f861

…d added missing spaces... to correct previous failing PR

dereuromark reviewed Jan 5, 2016
View reviewed changes

docs/README.md Outdated

Copy link

Owner

dereuromark Jan 5, 2016

Choose a reason for hiding this comment

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

overridden

dereuromark reviewed Jan 5, 2016
View reviewed changes

removed spaces after cases

5ae4c23

mtancoigne added 3 commits January 5, 2016 12:28

Reverting extra tabs

91b59b2

Fixed a typo

db95fae

Removed the bootstrap part on plugin::load

e21e2ff

dereuromark reviewed Jan 5, 2016
View reviewed changes

mtancoigne added 2 commits January 5, 2016 13:50

Updated plugin activation after @bravo-kernel proposition

b74b34a

And removed some extra spaces :)

750be22

dereuromark added a commit that referenced this pull request Jan 5, 2016

Merge pull request #18 from el-cms/master

dd45ad2

New configuration keys

dereuromark merged commit dd45ad2 into dereuromark:master Jan 5, 2016

New configuration keys #18

New configuration keys #18

Uh oh!

Conversation

mtancoigne commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

mtancoigne commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

bravo-kernel commented Jan 5, 2016

Uh oh!

dereuromark commented Jan 5, 2016

Uh oh!