MenuRenderer#allowed_child_classes #316

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@blj
blj commented Feb 8, 2012

Fix the ISSUE#316

MenuRenderer#allowed_child_classes throws error if allowed_children_cache is trimmed by db limit. This should ignore the missing classes and continue.

@jfahrenkrug
Member

Hi bij,

Thanks for your help. This is related to this ticket: #309
My question is: How come the field is still too small? That shouldn't be happening anymore with a size of 1500, right?

@blj
blj commented Feb 9, 2012

Hi jfahrenkrug,

Yes it is same as issue 309, but the problem is installations on databases of MySQL server before 5.0.3, which does not allow varchar more than 255 character length. This silently ignores the 1500 limit and sets it as 255. The best data type for this field is text type.

@jfahrenkrug
Member

@johnmuhl @saturnflyer What do you guys think? Are there any downsides to changing the type from string to text?

@blj
blj commented Feb 10, 2012

I personally do not see any problem in changing it to text datatype. I guess the main purpose of this attribute is to use in the admin UI, so even if the text field is slightly slower than varchar field types, it should not cause any problem with performance of the front end itself.

@johnmuhl
Member

mysql 5.0.x has been EOL'd. do we really need to support it?

@blj are there any major distributions that are still shipping pre-5.0.3?

@jfahrenkrug jfahrenkrug added a commit that closed this pull request Feb 10, 2012
@jfahrenkrug jfahrenkrug changed allowed_children_cache type from string to text so old (mysql…
…) versions dont truncate the contents and wreck havoc. Fixes GH-316
d87e9a7
@blj
blj commented Feb 10, 2012

@johnmuhl I am sure distributions have moved on, but the server administrators may not have. This is what happened to me couple of days ago. I moved my app to a new server using mysqldump which had preserved the varchar length 255.

@jfahrenkrug
Member

@blj are you sure that you hadn't set it up prior to this commit: 4404fb3 ?

@blj
blj commented Feb 10, 2012

If we do not want to change the data type, at least the error should be handled. Alternative solution is to work on a separate model for this which I do not mind doing tomorrow.

@blj
blj commented Feb 10, 2012

@jfahrenkrug it really does not matter when I have set it up, it only depends on the mysql server version that I have for the app.

@jfahrenkrug
Member

I'm just wondering if it's really MySQL's fault or if there's a chance that you migrated your DB prior to the commit I mentioned above.

@jfahrenkrug
Member

No, it does matter: I ran into the same thing, and the :limit option did the trick for me. Prior to that, I had your exact problem.

@blj
blj commented Feb 10, 2012

@jfahrenkrug It depends exactly on whether what was the MySQL version while running this commit

4404fb3

If it is anything less than 5.0.3, then it is not going to work. Text data type will work either way. Thanks.

@blj
blj commented Feb 10, 2012

I see that you have changed the datatype to text.

Also it is important the code is also changed to handle the missing constant. It is possible for people to remove an extension . The code as it is will throw error on a missing page class. The code that I did will address that problem and will silently ignores any missing constant.

@johnmuhl johnmuhl reopened this Feb 10, 2012
@johnmuhl
Member

the disabling an extension and getting missing page class error is something i've run into myself...so i'd like to have this just for that. unfortunately github won't let you merge after the PR has been once closed. i'll merge it locally and push.

@johnmuhl johnmuhl closed this Feb 10, 2012
@jfahrenkrug
Member

Thanks for your work @blj. I wasn't trying to say that you are wrong, I just wanted to make sure that the missing :limit option was not the culprit. Also sorry for closing this pull request too early :)

@blj
blj commented Feb 10, 2012

@jfahrenkrug cheers, no need to apologise :)

@blj
blj commented Feb 10, 2012

@jfahrenkrug I do not think it is a good idea to go back and change the column type in a previous migration. We will have to add a new migration file for any changes in the database.

I can clearly see now why my upgrade had missed the limit change. I am going to revert your commit and add a new migration on another pull request.

@blj blj added a commit to blj/radiant that referenced this pull request Feb 10, 2012
@blj blj Revert "changed allowed_children_cache type from string to text so ol…
…d (mysql) versions dont truncate the contents and wreck havoc. Fixes GH-316"

This reverts commit d87e9a7.

It is generally not a good idea to change the rails migrations. The changes may not be reflected for those who had run the db:migrate before the commit.
2a02679
@blj blj added a commit to blj/radiant that referenced this pull request Feb 10, 2012
@blj blj Changed allowed_children_cache type from string to text so old (mysql…
…) versions dont truncate the contents and wreck havoc. Fixes GH-316
88ac68c
@blj blj added a commit to blj/radiant that referenced this pull request Feb 10, 2012
@blj blj Revert "changed allowed_children_cache type from string to text so ol…
…d (mysql) versions dont truncate the contents and wreck havoc. Fixes GH-316"

This reverts commit d87e9a7.

It is generally not a good idea to change the rails migrations. The changes may not be reflected for those who had run the db:migrate before the commit.
c666e52
@blj blj added a commit to blj/radiant that referenced this pull request Feb 10, 2012
@blj blj Changed allowed_children_cache type from string to text so old (mysql…
…) versions dont truncate the contents and wreck havoc. Fixes GH-316
e9c6419
@saturnflyer saturnflyer commented on the diff Feb 10, 2012
app/models/menu_renderer.rb
@@ -35,7 +35,7 @@ def menu_renderer_modules
end
def allowed_child_classes
- (allowed_children_cache.to_s.split(',') - Array(excluded_class_names)).map(&:constantize)
+ (allowed_children_cache.to_s.split(',') - Array(excluded_class_names)).map{|name|name.constantize rescue nil}.compact
@saturnflyer
saturnflyer Feb 10, 2012 Radiant CMS dev team member

this shouldn't just rescue nil. That would rescue almost any exception. Can you change this to rescue from NameError and LoadError?

@blj
blj Feb 10, 2012

Yes, was thinking about this. Also do we add a message to the logger?

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