[PATCH] models in modules need .demodulize fixes for polymorphic associations

A plugin which fixes (monkeypatches) the issues has been added. There's another ticket on this here. My approach is similar, but should take care of all inconsistencies, association handling included.

The original issue discription is a bit off now, since the solution I'm proposing works differently, it now should read:

"The where clause in these cases gives conditions like: taggable_type = 'Container::Folder' which is what you'd expect."

Module names are now used as table_name prefixes, so Content::Folder.table_name will be 'content_folders'. The (last) module name will be singular, the class name will be plural by convention. This actually does break backwards compatibility, but it's an inconsistency that should be fixed once and for all. Luckily namespaced models haven't been encouraged in the past, so it might not be such an issue.

So are we moving to using demodulize on inheritance types, or towards not using it? I would prefer the latter.

We're not using demodulize on types from now on. That's how it should work and it truly allows namespaces, also at the db/table level.

Is this really a regression?

If so, we need a separate 1.2 fix which omits the backward-incompatible changes in addition to the patch for trunk.

We also need a migration generator to move folks from the old to the new type values.

And all of these changes need test coverage. Exhausting, I know :) Thanks loob.

I just wanted to point out that the changes only affect those who are already using namespaced models in their apps. Otherwise it won't affect anything. If any the improved table_name logic would be an issue, the fix is easy: just set the table_name manually. The only thing that's left is to update column values for the inheritance column; I can't see where a migration generator could help in this situation. You'd probably want to be selective on how and when you want to prefix the module name to those values.

I do understand the need for extra tests though, although the plugin already does some basic tests. There test/modules_test.rb file in AR is something I did before. Unfortunately by reusing the available tables/models Company, Client and so on makes things very hairy if you know what I mean. It leads to unnecessary complicated fixtures and associations. That's why implemented it as a plugin for now, to be able to test it seperatly.

I'd like to get a patch for this in if possible. What's the deal with providing backwards compatibility?

Is it fine to simply change the way AR works and provide example migrations or even a migration generator, or is backwards compatibility in this area a must for 1.3? This could be done with a flag set on AR::Base to toggle between the two behaviours.

It should be noted that the necessary changes will only affect people already storing the names of namespaced models in their applications. I think most people would be unaffected.

I've whipped up an improved version of the namespaced_models plugin posted here.

http://svn.viney.net.nz/things/rails/plugins/namespaced_models

Merging the changes into Rails isn't a big job, but fixing all the test cases is the sort of task you wouldn't wish on anybody. It may be simpler to rewrite them with new fixtures, rather than try and rework the existing ones.

+1 for this patch

I had this idea while working on #8740 and figured it might be useful in this ticket too.

To ease the transition, how about you add some smarts to compute_type / type_name_with_module, so that they'll work with both modularized and demodularized type values. (while defaulting to modularized).

A migration is still welcome as having inconsistent records would not be wise in the long term.

Test me! :)

Replying to jonathan_viney:

I've whipped up an improved version of the namespaced_models plugin posted here. http://svn.viney.net.nz/things/rails/plugins/namespaced_models

This plugin makes my namespaced STI models functional. Namespaced STI is unuseable in Rails 1.2.3 and 2.0.2 (and everything in between I'd guess) without the plugin because the model class name and the value stored in its inheritance column do not match.

+1 for incorporating this much needed fix

Like the commentors before me, I leave the ticket closed but the issue isn't closed at all. It's a valid, current major defect.

This ticket is *not closed* -- the patch has no tests, so the ticket is marked as untested. To incorporate this change, please add unit tests for it!