fix JModelLegacy __construct method to determine option #1500

Merged
merged 3 commits into from Oct 10, 2012

Conversation

Projects
None yet
7 participants
Contributor

dongilbert commented Aug 28, 2012

Line 216 of JModelLegacy will incorrectly parse your model class name if your model class ends in model. I had a situation where I was building an auto manager for a client, and one of the models was named AutosModelModel. The current regex parsed the classname and thought that the option was com_autosmodel, because it doesn't take into account the possibility that a model class can end in model.

This pull request address issue #1499

Note: This fix breaks backwards compatibility (potentially) because previously the regexp used the /i modifier, searching for case-insensitive strings. This new implementation uses case-sensitive strpos. Make sure your class names are properly CamelCased. (http://docs.joomla.org/Coding_style_and_standards#Classes)

Contributor

ianmacl commented Aug 29, 2012

I'm fine with this if somebody from the CMS can stamp their approval on it.

Contributor

dongilbert commented Aug 29, 2012

I have a pull request pending there as well as a tracker item.

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29015

Contributor

elinw commented Aug 31, 2012

Could we also do the same for view?

Contributor

dongilbert commented Aug 31, 2012

Where are you referring to make this change? The only thing close I can find is https://github.com/dongilbert/joomla-cms/blob/master/libraries/joomla/application/component/view.php#L547

Is that what you mean?

Owner

mbabker commented Sep 1, 2012

In the current Platform code, the method is here: https://github.com/joomla/joomla-platform/blob/staging/libraries/legacy/view/legacy.php#L444

Fixing the regex for JViewLegacy should hopefully allow us to rename this CMS class to the proper name: https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_templates/views/prevuuw/view.html.php#L19

Contributor

dongilbert commented Sep 1, 2012

Thanks for the links. I'll work on that. Should it be a separate pull request though? I think so, as what's implemented here is functioning independent of the view regex.

Contributor

dongilbert commented Sep 1, 2012

@mbabker @elinw - can you test this? https://github.com/dongilbert/joomla-platform/compare/jviewgetname.diff

I've done some testing and received expected results.

This is how I tested - https://gist.github.com/3563618

#1509

Contributor

pasamio commented Sep 2, 2012

Actually the current regexp supports TemplatesViewPreview just fine, it's been fixed for a while just the view hasn't been renamed. You can test it and see.

Contributor

pasamio commented Sep 2, 2012

Ok, I double checked while the regexp supports it, the check if the view name that we found includes view is what kills it for some reason.

Contributor

dongilbert commented Sep 3, 2012

This discussion got a little sidetracked from this pull request to another one concerning JViewLegacy. What's everyone's thought on this one?

Contributor

eddieajau commented Sep 3, 2012

Is there any reason why you can't remove the regex and use the same technique as in JViewLegacy?

Contributor

dongilbert commented Sep 3, 2012

Ya - I thought that same thing and just pushed it.

@dongilbert dongilbert fix JModel __construct regex to determine option
dropped regexp in favor of strpos

Updating JModelLegacy::getName with same technique
0a7653a
Contributor

dongilbert commented Sep 3, 2012

I also updated the getName method here with the same technique. Squashed and pushed last 3 commits

Contributor

dongilbert commented Sep 6, 2012

I've been testing this locally with no issues, but I would like (for obvious reasons) some others to test as well. Any takers?

Contributor

elinw commented Sep 6, 2012

It's one of those unfair situations because it's not your fault that there are not tests (and if there were someone would have thought about doing this a long time ago) but if you could do a little bit of test writing that would be awesome.
https://github.com/joomla/joomla-platform/blob/staging/tests/suites/legacy/model/JModelLegacyTest.php

@pasamio now you know why I asked about view (even though we pulled that view anyway).

Contributor

dongilbert commented Sep 7, 2012

Gotcha - I guess my first go at unit testing is about to take place. :) I've read about it a lot, but haven't created any unit tests yet. Here's my shot.

Contributor

LouisLandry commented Oct 9, 2012

@dongilbert for what it's worth I really like the change. I look forward to reviewing this when you get the tests sorted out. If you need help getting those done please feel free to ask either on list or in this pull request any questions you may have.

I'm going to close this request for now while we wait for you to get those tests sorted out. Please note that this isn't due to rejecting the request, but just a matter of keeping our pull request queue free of things not actively needing review. When you get the unit tests sorted out just re-open the request and we'll go from there. Thanks a bunch for the initiative and the work.

LouisLandry closed this Oct 9, 2012

Contributor

dongilbert commented Oct 9, 2012

I've been struggling with the tests, as I wasn't sure if I should use mock classes for the model, or just create some class name strings to use.

Also, I just set up a new computer, and don't have phpunit installed yet. I was going to install via composer, because I didn't really want to install the pear library. However, I don't think PHPCS has composer support yet, so I may have to install pear after all.

Contributor

dongilbert commented Oct 9, 2012

Also, what PHPUnit optional packages are needed for testing the Joomla Platform

Contributor

LouisLandry commented Oct 9, 2012

I tend to just set mine up with the directions at: http://jenkins-php.org

That gets pretty much the full compliment of tools I use for static analysis and testing (including PHP_CodeSniffer). As for dependencies the only think I can think of that you may need to install extra is phpunit/DbUnit. I don't remember if it comes packaged or not.

You can follow the instructions in the readme at: https://github.com/joomla/coding-standards to install our standard once you have PHP_CodeSniffer installed.

Contributor

dongilbert commented Oct 9, 2012

Thanks Louis - that helped.

So what are your thoughts on creating the tests - Should probably just use strings of potential class names for testing, rather than mock models, right?

Also, this should only include tests relative to the pr, instead of the full on JModelLegacy testing that still needs to be completed. (Which I think, since I'm in there, I'll probably complete)

Contributor

LouisLandry commented Oct 9, 2012

I think you are on the right track. One of the principles of good unit testing is to really isolate the exact "unit" of code you are testing. In this case really you are wanting to test the class name evaluation, so if you can avoid having to create a whole bunch of test classes, etc then that is the preferred path.

For this pull request we only really need tests for the changes being made on this pull request. If you wanna have a go at the rest, my suggestion is to do them separately in a separate branch so as to not let that work get in the way of getting this stuff done and merged. That being said, if you want to just go for it that is entirely up to you.

Contributor

dongilbert commented Oct 9, 2012

I can't run the test suite against the legacy tree, not sure what's going on there - I may have borked my installation by merging joomla-platform/master

Contributor

dongilbert commented Oct 9, 2012

It doesn't appear as though PHPUnit is running the legacy test suite. It's not specified in the phpunit.xml.dist

Contributor

LouisLandry commented Oct 9, 2012

Try running phpunit -c legacy.xml.dist to have it run the legacy suite.

Contributor

dongilbert commented Oct 9, 2012

That works much better! Thanks - Now I can start actually writing the tests.

Contributor

dongilbert commented Oct 9, 2012

I've got the tests ready, how do I reopen a pull request? Just send in another one?

Contributor

eddieajau commented Oct 9, 2012

Should be a "Reopen" button at the bottom of the page?

Contributor

LouisLandry commented Oct 9, 2012

Also, you need to merge your new commits into the jmodelfix branch and push it up to github so that they show up in this pull request.

Contributor

dongilbert commented Oct 9, 2012

I don't see one. It's supposed to be next to the comment button, right?

Sent from my iPhone

On Oct 9, 2012, at 5:40 PM, Andrew Eddie notifications@github.com wrote:

Should be a "Reopen" button at the bottom of the page?


Reply to this email directly or view it on GitHub.

Contributor

dongilbert commented Oct 9, 2012

I thought I did that. Maybe I pushed it tithe wrong branch. You can see it in the diff I linked too.

Sent from my iPhone

On Oct 9, 2012, at 5:42 PM, Louis Landry notifications@github.com wrote:

Also, you need to merge your new commits into the jmodelfix branch and push it up to github so that they show up in this pull request.


Reply to this email directly or view it on GitHub.

Contributor

dongilbert commented Oct 9, 2012

I double checked and I did push it to the correct branch. Also, there is definitely not a reopen button at the bottom of this page.

Contributor

eddieajau commented Oct 9, 2012

So, turns out only maintainers can re-open sigh Best laid plans of mice and men and all that :) Re-opening.

eddieajau reopened this Oct 9, 2012

Contributor

dongilbert commented Oct 9, 2012

Ya. Funny. I wonder if after we build this new issue tracker if we could build in the ability for users to submit an issue for re-opening.

The tests have been completed and added. I'm going to add the rest of the tests after this has been verified. ( since its my first experience with unit tests)

Contributor

LouisLandry commented Oct 10, 2012

All passes! Thanks a bunch for your patience and work through this @dongilbert.

@LouisLandry LouisLandry added a commit that referenced this pull request Oct 10, 2012

@LouisLandry LouisLandry Merge pull request #1500 from dongilbert/jmodelfix
fix JModelLegacy __construct method to determine option
6d6c331

@LouisLandry LouisLandry merged commit 6d6c331 into joomla:staging Oct 10, 2012

Contributor

dongilbert commented Oct 10, 2012

Not a problem at all - I understand the difficulties.

I'm going to create a new branch and finish the unit testing for
JModelLegacy. I've heard from other develpers that once you get into unit
testing your apps, it's hard to stop. It's like an addiction.

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