tingobol / gii-template-collection

Automatically exported from code.google.com/p/gii-template-collection
0 stars 0 forks source link

accessRules and filters should be removed #2

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I've been analyzing many applications I've developed with Yii (iCrud and GTC) 
and I've noticed that filters() function and accessRules() function could be 
removed from the Controller class template, because many users (like me) use 
another module for managing the accessRules (eg: Srbac, hrbac, and similars). 
So, I think, accessRules() function should be removed. In the case of rules() 
function, I think it should be something like I show below because the 
accessControl filter wouldn't be used:

    public function filters()
    {
        return array(
        );
    }

What do you think guys?

Original issue reported on code.google.com by robregonm on 2 Aug 2010 at 8:17

GoogleCodeExporter commented 9 years ago
Also, I think that the translations of the fields in attributeLabels() function 
in model.php shouldn't use 'app' messages, for example, it should be something 
like this:

public function attributeLabels()
{
    return array(
        'id' => Yii::t('gtc', 'ID'),
        'description' => Yii::t('app', 'description'),
        'volume' => Yii::t('app', 'volume'),
    );
}

So, I think it should be a good idea to rename the messages file to gtc.php, 
and use app.php for application messages and not for gtc messages. What do you 
think?

Original comment by robregonm on 3 Aug 2010 at 3:27

GoogleCodeExporter commented 9 years ago
Regarding the filters.. I do agree with you that one almost always end up 
modifying the filters or using something else than accessRules(). However, I'm 
not sure that we should remove it and thereby end up with CRUD controllers that 
does no access checking at all?

I mean; Sure it is simple for the user to add this themselves, but it makes the 
generated package somewhat incomplete, and in a way it's also slightly insecure 
that there is no access control.

So to sum it up; Unless we *replace* the standard filters with some other 
access control, I think I'm a little reluctant to the suggestion.

Also, the suggested change is essentially about removing one method in the 
class, and one line in filters(). If the user wants to do that it's not a lot 
of work, so I'm not sure there's much to gain? Please correct me if I'm missing 
something obvious here.

Original comment by l...@finalresort.org on 4 Aug 2010 at 10:47

GoogleCodeExporter commented 9 years ago
Regarding the translation (a new issue next time! ;D), can you elaborate on why 
we should break out the model's translations? I mean, what does doing so 
contribute to the code?

Original comment by l...@finalresort.org on 4 Aug 2010 at 11:10

GoogleCodeExporter commented 9 years ago
The column labels are particular for every web application, however, we can 
provide a basic kit of fields (ID, description, name), for that reason we 
should generate something like I suggest above.
When I developed iCrud, I name the messages package as App because every webapp 
should include a translation package (I didn't think that it has evolved into 
something more powerful like GTC is right now ;) ), however, right now, 
considering now we are translating the column labels too, I think GTC should 
rename the messages package to GTC (or something like that), and keep "App" for 
the internal application use. That way, every developer can include their own 
translation messages without modifying GTC messages.

Original comment by robregonm on 4 Aug 2010 at 11:58

GoogleCodeExporter commented 9 years ago
i think robregonm is right. To have much smaller, categorized translation files 
is better than having one big. i think i will clean this up piece for piece 
after time...

to the filters: i think the web application generated by FullCrud should have 
as much features as possible, so developer just needs to _remove_ unwanted 
things afterwards. this is faster than needing to copy&paste much needed code 
from somewhere. On the long term, i think it would make sense to use different 
templates. One template 'classical Yii Access Filter', another one 'Srbac' and 
so on. 

after all, it should be a collection of cool templates, and fullCrud is just 
one of them. the one with the most possible features availabe - FULL ;)

i vote for keeping the filters() and accessRules as they are.

Original comment by thyseus on 4 Aug 2010 at 8:04

GoogleCodeExporter commented 9 years ago
Filters: I too think that they should be there. It's very simple to remove 
those two functions if wanted (search/replace over these few files and it's 
done in less than a minute).

Translation files: If the two of you think that the translations should be 
split into seperate parts, go for it. As I understand it you want to seperate 
the translations for models, but shouldn't also the ones from other parts such 
as the relation widget and the rest of CRUD also be seperate parts then? So for 
example the categories 'model', 'crud', 'relation'.

I dunno, I feel it's a bit overkill, but again, do it if you feel like it :)

Original comment by l...@finalresort.org on 4 Aug 2010 at 8:15

GoogleCodeExporter commented 9 years ago
Based on the affirmation of having a FULL Crud, It is a good idea of keeping 
the two functions. My Vote for keeping.
I like the idea of having many templates based on "Classic Yii Access", 
"Srbac", and so on.... I vote for this too.
And also I vote for renaming the current translation packages to "GTC" or 
something like that, and reserving "App" for the developer use, and we just use 
the reference to it. However, I do like the idea of splitting the translation 
package into many files as Leo say, because, many messages are used in many 
views, and into the controller... and inside the model.... My vote is for 
renaming and reserve App for webapp use.

Original comment by robregonm on 4 Aug 2010 at 9:50

GoogleCodeExporter commented 9 years ago
Already fixed

Original comment by robregonm on 3 Sep 2010 at 4:01