For anyone looking at this, I think that JControllerLegacyTest::testSetRedirect() lines 486 to 574. The 88 lines of code and the 33 asserts are a good indicator, but I'm too familiar with PHPUnit testing yet (working on it.)

How would I go about changing the test?

Do we have a joomlacode tracker for this?

Infograf768, I made a tracker for the fragile unit tests. #31153

Fantastic work on this, now let's see if we can find why the tests are failing.

Where are the unit tests used by the travicCI taken from? If these its from the current Joomla repository, then this merge will fail. Now that I think about it I shouldn't have edited the functions until this was merged.

Travis tests the branch the pull request is based on. So it's testing your branch with the modified code and tests.

Thanks!

Overall: Very good job

I only have some minor observations to make.

libraries/legacy/controller/admin.php

• lines 97 to 100: Why are you handling the exception if you are simply throwing it back? Just add a comment above line 92 stating that this may throw an exception.
• line 222: Maybe you meant $msg = $this->getPublishResultMsg($value); Otherwise I can't understand line 223
• getPublishResultMsg: Please fix the docblock :)

libraries/legacy/controller/legacy.php:

• What is the point of setModelPrefix? It doesn't simplify the code or remove any redundancy. Plus, I find it bad form to use $this->someMethod() from an object constructor.

Thanks for starting this. I Agree with @nikosdion that it is not beautiful to use a method of an object in its constructor (kind of: eating the fresh crust from the bread you are about to put in the oven). In some languages it would give problems, although it is perfectly valid in PHP.

BUT it is unnecessary here, I think: in the constructor $this->view_list and $this->model_prefix are always empty, aren't they? So the outer conditional can be put away and the extra method is not necessary. OOOOPS, sorry, not true: in a derived class $this->view_list and $this->model_prefix can be set and if the parent class is called after that that setting should not be overriden.

About names: I wouldn't call a variable $xxxArray. I think that is what Uncle Bob calls "avoid encoding in names". Your $classNameArray is also not an array of clasnames, but an array of parts of the classname. So I would call it $classNameParts. I would call your $cidArray $cids (or a longer, more explanatory name... what does that letter 'c' mean?).

The naming of $viewClass is also not completely clear. Do you mean $viewClassName? Unfortunately some variable-names cannot be refactored, because they are allready used in a lot of derivated code. So it will always be a compromise: finding the best names without breaking backwards compatibility.

@nikosdion

Thanks for the feedback regarding your comments
line 97 ~ 100 This was from the original constructor method I was trying to keep the behavior as close to the original as possible, but you're right it isn't needed. It also saves some nested brackets.

line 92 the try statement is a good sign that an exception is going to be thrown. Personally I don't like adding obvious comments to the code because it just gets in the way. But if we remove the try then commenting would make sense.

line 222: Yep that is what I meant. Thanks

getPublishResultMsg docblock : consider it done. =^D

legacy.php

I moved the set model prefix out because then it can be overridden and it also gets rid of a nested conditional. I don't really understand the negative consequences of using class methods in the constructor. I've always done it because A: I only write the constructor after I've written all the functions and B: I've always felt the constructor should have to adhere to the interface too, albeit loosely since it can access protected an private functions too.

Then when someone wants to change the way the constructor works they don't have to override the constructor (Yuk) they can just override the function that the constructor calls on. Of course that is just my opinion and since I'm a DIY coder with no University exp, I might have missed something. If that is the case please teach me, because I defiantly know there are gaps in my understanding, just not where they are.

@ HermanPeeren

I've always wondered what the c means in cid, but I can see your point about the variable encoding. I'll re-factor accordingly

As for the $viewClass thought about naming it $viewClassName, but since its used in the new $viewClass() statement which wouldn't make sense if we went new $viewClassName() I went for the former.

Thanks again to both of your for the feedback. It is really refreshing to hear how others Joomla!-ing

This needs to be updated. Constructors in PHP are really designed to be overridden by subclasses, that's why it's the one place where signatures don't have to match. It's better to override them than the methods.

The C in cid in core Joomla almost always refers to the category id for the content itm.

@elinw

Sorry for the late response. Thanks for the comment. The only question I have is why do we want to include a reg expressions in the constructor in the first place?