Hi everyone. I was hired by a company to refactor one of their main systems built on top of CI. I spent 3 months fixing CI bugs and re-designing the entire architecture of the framework. I was quite shocked to find out so many abnormalities and design errors.
Here’s a list of the ones that I consider critical and I think everyone should be aware of:
1. 90% of the system is written in PHP 4, a deprecated version of the language.
2. The framework doesn’t have a Front Controller. Strangely, the application controller becomes the Front Controller, this means that the framework can only load one controller at a time (per request).
3. The CodeIgniter/CodeIgniter.php file is not a class, it should be part of the bootstrapper file, index.php.
4. MyController > Controller > CI_Base. There’s no point of having 3 levels of inheritance. The only purpose of CI_Base is to return a single instance of the Controller, so it should be part of the Controller.
5. Namespaces: The Controller class doesn’t follow the naming convention, it’s not prefixed with CI_ like the rest of the core classes. This makes CI’s namespace support very unreliable.
6. The application controller acts as a Front Controller and Registry, it’s used to stores all the objects the framework needs.
7. Circular reference: When the Front Controller (App. Controller) loads a model, the framework assigns to the model all the object properties of the Front Controller. This is the most obscure behaviour I found in CI.
8. The router.php file contains an array that uses the keys to store data and define regular expressions. If you define 100 elements, the Router class will loop through the entire array, extract the keys and execute 100 regular expressions.
9. Common.php is not a class, it should be moved to the helpers/ directory and renamed to something more appropriate, like mvc_helper.php for example.
10. Very poor OOP and almost no design patterns makes this framework very difficult to extend and maintain.
Seriously though, you’ll love Kohana and its community. Their main developers feel pretty much the same way about CI and have already “fixed” most of its “abnormalities”.
Hi buda ,
This is a nice issue you are foucusing.I am a CI fan . I dont want to switch from CI to other framework. I hope in the next version of CI may be CI 2.0 this issued are should be resolved. Because PHP 4 is died almost. Also there should be support naming conventions.
1. 90% of the system is written in PHP 4, a deprecated version of the language.
If you are going to support php4 and want to have the smallest code base you write in php4 because php5 is backward compatible? How would you create a version independent codebase?
Buda - 14 March 2008 07:58 PM
2. The framework doesn’t have a Front Controller. Strangely, the application controller becomes the Front Controller, this means that the framework can only load one controller at a time (per request).
How many controllers do you need per request?
Buda - 14 March 2008 07:58 PM
3. The CodeIgniter/CodeIgniter.php file is not a class, it should be part of the bootstrapper file, index.php.
Can’t it be because they want to keep the the actual framework code together? If you don’t want to load the default codeigniter file you could make a custom one and load that in the bootstrap file instead of creating different bootstrap files.
Buda - 14 March 2008 07:58 PM
4. MyController > Controller > CI_Base. There’s no point of having 3 levels of inheritance. The only purpose of CI_Base is to return a single instance of the Controller, so it should be part of the Controller.
Maybe this is a window to improve the framework?
Buda - 14 March 2008 07:58 PM
6. The application controller acts as a Front Controller and Registry, it’s used to stores all the objects the framework needs.
I agree there should be a registry to store different objects in their own realm.
Buda - 14 March 2008 07:58 PM
7. Circular reference: When the Front Controller (App. Controller) loads a model, the framework assigns to the model all the object properties of the Front Controller. This is the most obscure behaviour I found in CI.
Why is this bad?
Buda - 14 March 2008 07:58 PM
8. The router.php file contains an array that uses the keys to store data and define regular expressions. If you define 100 elements, the Router class will loop through the entire array, extract the keys and execute 100 regular expressions.
If a match is found the loop stops.
Buda - 14 March 2008 07:58 PM
9. Common.php is not a class, it should be moved to the helpers/ directory and renamed to something more appropriate, like mvc_helper.php for example.
It will be for the same reason as the codeingiter.php file to keep the framework code together.
Buda - 14 March 2008 07:58 PM
10. Very poor OOP and almost no design patterns makes this framework very difficult to extend and maintain.
How is it difficult to extend the framework the forum is filled with extensions.
If you are going to support php4 and want to have the smallest code base you write in php4 because php5 is backward compatible? How would you create a version independent codebase?
You wouldn’t. You would just make it PHP5.
xwero - 14 March 2008 11:04 PM
How many controllers do you need per request?
One never knows. But there are plenty of use cases for forwarding to another controller action or needing to call multiple actions. Extending CI to do this would not be difficult.
xwero - 14 March 2008 11:04 PM
Can’t it be because they want to keep the the actual framework code together? If you don’t want to load the default codeigniter file you could make a custom one and load that in the bootstrap file instead of creating different bootstrap files.
For security reasons, index.php should contain as little code as possible. I think the current implementation is correct in this regard.
xwero - 14 March 2008 11:04 PM
Maybe this is a window to improve the framework?
It would be easy to make a singleton in PHP5. Even better to have a proper front controller as singleton that dispatches to action controllers.
xwero - 14 March 2008 11:04 PM
I agree there should be a registry to store different objects in their own realm.
Easy to do. A registry should be a singleton, so again, PHP5 is best here.
xwero - 14 March 2008 11:04 PM
Why is this bad?
It eats up resources.
xwero - 14 March 2008 11:04 PM
If a match is found the loop stops.
If a match isn’t found, it doesn’t. Who has 100 routes though? Still, regex eats up resources.
xwero - 14 March 2008 11:04 PM
It will be for the same reason as the codeingiter.php file to keep the framework code together.
The architecture could be better.
xwero - 14 March 2008 11:04 PM
How is it difficult to extend the framework the forum is filled with extensions.
An improved architecture would def. make the framework more user friendly.
If you are going to support php4 and want to have the smallest code base you write in php4 because php5 is backward compatible? How would you create a version independent codebase?
You wouldn’t. You would just make it PHP5.
May i be a little evil now? If you have a business and you run 50 sites on php4 economics come before technology. You are not going to change the code to php5 because someone says hey php4 is death. 50 sites means many hours without making money. The move will happen but technology business is not as fast as the technology it runs on because they have to put food on the table.
thurting - 15 March 2008 02:47 AM
xwero - 14 March 2008 11:04 PM
Maybe this is a window to improve the framework?
It would be easy to make a singleton in PHP5. Even better to have a proper front controller as singleton that dispatches to action controllers.
xwero - 14 March 2008 11:04 PM
I agree there should be a registry to store different objects in their own realm.
Easy to do. A registry should be a singleton, so again, PHP5 is best here.
You can have a singleton in php4, that is a false reason to move to php5.
thurting - 15 March 2008 02:47 AM
xwero - 14 March 2008 11:04 PM
Why is this bad?
It eats up resources.
If i’m not mistaken the properties are added by reference so it doesn’t eat up resources.
thurting - 15 March 2008 02:47 AM
xwero - 14 March 2008 11:04 PM
If a match is found the loop stops.
If a match isn’t found, it doesn’t. Who has 100 routes though? Still, regex eats up resources.
What would be a better way to have custom routing?
thurting - 15 March 2008 02:47 AM
xwero - 14 March 2008 11:04 PM
It will be for the same reason as the codeingiter.php file to keep the framework code together.
The architecture could be better.
xwero - 14 March 2008 11:04 PM
How is it difficult to extend the framework the forum is filled with extensions.
An improved architecture would def. make the framework more user friendly.
Ok CI is not the perfect framework. Use the framework you feel most comfortable with. About the user friendliness of the framework, there are many people who come from different backgrounds and even other php frameworks that find CI easy to use. i think that is all that needs to be said.
If your sites are using PHP 4, that’s fine, no need to upgrade, move to a different server or refactor the code. But if you are still developing using version 4, that’s very irresponsible and unprofessional, considering that you are selling to your clients a software developed using an obsolete technology. Also, my post it’s not about “upgrade to php5”, it’s more about “you can improve some aspects of the framework you are using”.
@thurting
Thanks, I think you answered some of xwero’s questions up there.
Being able to call multiple controllers, to handle different types of request and forward them, is the basics of any framework and the first step to achieve true modularity.
php4 and php5 is not about singletons, is about quality assurance, performance, scalability. A clear example is CI’s Model class, when instead of using the __get to check if a class member exists, it loops through the Controller’s object properties and assigns them to the model. So if you load 10 models, you’ll end up having 10 Front Controllers, 10 Registries and 10 Models. That’s wrong, and it’s a very serious issue, because it leads to circular reference and makes the system very difficult to debug and maintain.
Ok CI is not the perfect framework. Use the framework you feel most comfortable with.
Again please, don’t get me wrong, I’m not here to compare or talk about other frameworks. I don’t use CI, but you do. So this might help you (and other developers) understand a little bit more the framework you are using.
NOTE: I listed only 10 of all the design problems I found, there are more than 50. I spent 3 months documenting all this issues, writing unit tests and UML diagrams.
There is too much Bla bla bla here. If you really make improvements, then post the code that you said you have improved or open a CI’s fork website so you can post there your full remastered CI. Once we see code then we also can discuss better.
There is too much Bla bla bla here. If you really make improvements, then post the code that you said you have improved or open a CI’s fork website so you can post there your full remastered CI. Once we see code then we also can discuss better.
If you want to see the code, download the framework, it’s all there. The info I posted will help you find this issues. For example:
Design errors
Loader.php, line 177
$CI->$name = new $model(); $CI->$name->_assign_libraries();
That’s when all the core libraries are assigned to a model, one of the most obscures behaviours I’ve seen so far.
CodeIgniter.php, line 97
if ($EXT->_call_hook('cache_override') === FALSE)
Some methods are prefixed with the private/protected underscore, but called as if they where public methods from anywhere. By doing this, developers cannot relay on the naming convention of the system, making the API almost useless.
Base5.php, line 55
function &get;_instance()
This file mixes a class with a function. That’s is wrong, but worst is Base4.php:
Base4.php, line 41
class CI_Base extends CI_Loader {
The Application Controller extends the Controller class, which extends the CI_Base class. Now, normally applications don’t have 3 levels of inheritance, but Base4.php goes a step further and extends a 3rd class, CI_Loader, that has nothing to do with the Application Controller itself.
Coding errors
class_exists('CI_DB')
I found a lot of coding errors, for example, the use of class_exists() function, where the second parameter is missing. This is a mayor bug in CI. Then, the unnecessary use of inlcude() instead of inlcude_once(), and more.
If you really make improvements, then post the code
I was hired by a company to perform a code review and refactor the framework. Everything I did with CI belongs to my employer and I’m not planning to post any of my employers code, that goes against my legal responsibilities.
I’m not here to talk about other frameworks or to debate about architectural or software development issues. I’m here to post some CI related issues, some people might find it useful.
@Buda : I like your look at the framework but i found thurtings answer a bit condescending. that’s why a reacted a bit sharper than i normally do. i think your analysis could open the eyes of the people at Ellislab because you really took the time to find out if CI is a good framework to base all other code on. I can only applaud you for sharing your knowledge.
Buda is spot on across all 10 points. I experienced the exact same limitations and had to code in my own solutions whilst trying to leave as much of the core in-tact. I wasn’t aware of Kohana, does Kohana allow more modular controllers as buda mentioned as that was the main issue I had to solve.
Also will kohana last, CI is after all backed by a company entity.
Buda, can you give us pseudocode or an outline of how you handled the Frontcontroller? I’d be interested in seeing your approach. On the ten points you raise, I must say I agree with you on basically all of them.
I must admit that I enjoy Kohana a lot nowadays and I’m confident it will last. CI might be backed by a company but I don’t think this has reflected on the codebase the last year or so. The last two releases didn’t solve the major issues I had with CI and when I look at the recent SVN history it’s mostly typofixes in the documentation. Compare that to the Unit Testing module added to Kohana only days ago. I’m confident though that with the release of ExpressionEngine2.0, development for CI will pick up.
CI for me has certainly opened up my eyes for proper web development but honestly I have moved on to more advanced code and CI held me back. However, I could not have done it without CI and I wish it all the best in the future (the jQuery library Derek showed off somewhere looks really neat)
