CraftCMS is the main product of a small, Oregon (USA) based, software company named Pixel & Tonic. It’s a master example of how bad source code can be turned into a successful product. In this blog post I’ll cover some of the technical issues. I’ll show you some examples of bad development practice and problems CraftCMS and Craft Commerce are currently facing.
Popular software does not always has to be well-written. Maybe that’s exactly why a software project is successful at the first place, because programmers do not only focus on well-written code, but rather getting stuff done. As a programmer who works for a company which has time pressure, I understand that. Focusing more on getting features done, even if this means the software has bugs. A common practice in the software industry. The client wants to see results. Project managers pushing developers to have something to show in their next project meeting.
Software written under time pressure is often bad. But you cannot always skip the thinking process. You have to think longer and spend time on solutions. Even under time pressure.
CraftCMS is a perfect solution for frontend developers. They do not have to deal with PHP code. Only creating Twig templates and using existing Craft functionality to query data out of the database. This is fine as long as you don’t have any special cases.
It’s getting dirty when you want to extend the system and write own PHP code. My request to get more documentation for the PHP backend provided has been rejected several times. [1][2] As you can see in GitHub Issue #621, I got forwarded to some core source code instead of a proper backend documentation. When I have a problem to solve, random code from the core does not help me. I don’t know which parts of the shown code are relevant for me. I would like to have better tutorials and best practice documentations for how to solve specific PHP problems. I would not expect to get forwarded to some random position in the PHP core of CraftCMS.
Caching
It’s mixed how CraftCMS uses caches. The usage of caching techniques differ from one end of the system to the other. At some points CraftCMS forces you to call a refresh function. Like when you want to get the new inserted field it forces you to call Craft::$app->getFields()->refreshFields() [3] first.
At some other points it does not use any caching at all. There is one good example how you should not do it. Let’s dig into Sections::getSectionByHandle() [4].
What does it do? On the very first call it gets all sections from the database by calling $this->getAllSections() [5]. All the sections will be forwarded to a common array helper function called ArrayHelper::firstWhere() [6]. Then this helper function goes through all rows and uses its arguments to find the desired row. It does this for every request. So, every time you press Reload in your browser, CraftCMS is going to fetch all rows from the table. Unlimited. Imagine the table has thousands of rows, going (in the worst case) through the whole table causes unnecessary PHP workload, and unnecessary traffic between the database and PHP. Instead a real database query using a WHERE clause should be used.
In fact the getAllSections() function uses a caching variable, but it is useless since every new request will cause PHP to fetch again all rows from the table.
Beside using ArrayHelper::firstWhere() is a bad idea, it’s also faulty by design. Have a look at the function arguments. There is one boolean argument named $strict. It’s bad design to use one argument to change the behaviour of a function. Instead using a second function named ArrayHelper::strictFirstWhere() would be better design. But anyway, the whole idea of first fetching the whole table and then filtering it in PHP should be considered as bad.
This is only my opinion. I asked Reddit and Stack Overflow about this topic in an abstract way. The answers vary from one platform to the other. You should read them and form your own opinion about it.
Installation
I have never done a CraftCMS installation on the first try. It never worked on the first try. Every time I install a new instance an error appears I never saw before. The setup script does not recognize existing variables from the .env file. Run the setup 10 times (for the same instance) and you have to provide the password and other variables 10 times. Further, do not forget to delete the entire database each time you restart the installation. This is a pain in the ass. Nothing else to say.
Project Config
Project Config is one useless features of CraftCMS version 3.1. This feature is not designed to be used by many developers at a time. The background logic, how this feature is actually working, is not clear. The changes from the project.yml file are often not applied to the other environments. Even when changes are made in this file and the Modified Timestamp is updated. This caused us too many headaches.
You may first think that this will help you manage the fields in a better way. But when managing large sets of fields and settings and plugins project.yml becomes a nightmare.
Different values in different environments are required. This is the whole point of using environments. Not to override the same value on all environments, Craft introduced a feature to replace the value of a config field with an environment variable. For instance, instead of subject: 'Hello World' you can now use subject: '$SUBJECT', where $SUBJECT comes from the .env file or the shell environment. This will set the value of the environment variable $SUBJECT to the YAML subject variable. This is cool, it seems on the first glance. Or not so when using it in real world. Because now I would expect also subject: 'TEST - $SUBJECT' to work. It does not by intention.
The Project Config concept is faulty by design. Instead of first changing something in the Control Panel and then commiting the project.yml file to Git, it should be, first editing the project.yml file in a text editor, then run the sync script and commit the file to Git. Without any magic in the background. Things I change in the Control Panel should never affect files. Since we use Docker, the next deployment will overwrite the changes. It’s one thing to achieve real good magic and another trying to make magic and failing like CraftCMS with Project Config.
Keep in mind that this is no replacement for Migrations. It only overrides existing values. Often it does not work how expected.
Craft Commerce
The same company, Pixel & Tonic, also produces an e-commerce solution call Craft Commerce.
The majority of the Craft users are not going to the edge. This is the 80/20 rule: 80% of the user using 20% of all features. We discovered a 1.5 year old bug in Craft Commerce. No one ever used this feature or tested it in the moment of creating the code for it. Otherwise the developer would have seen that the table does not exist. This bug was only introduced because the Pixel & Tonic developers are not willing to reuse functions. On each position where table names are needed, they write the full table name instead of calling the function which returns the table name. They argue with performance reasons. But the truth is that you should not use PHP to be performant. You should not care about performance when writing PHP code. You write PHP code because you don’t want to care about memory management and the other low-level stuff which is needed to be real performant.
A short background. CraftCMS uses the Yii2 Framework as a backbone. Active Records will be used to fetch data from the database. Each Active Record has a static function to get the database table name.
Instead of using this static function for each extra position where the table name is needed, the CraftCMS code is designed to not reuse the tableName(). Why? This is against Don’t Repeat Yourself (DRY). Pixel & Tonic is acting against Developer Best Practices.
The unit tests are not going to catch such kind of bug, because there are no unit tests at all. No unit tests for a system which deals with numbers and billing and invoices for end-users.
See an example of wrong numbers handling in GitHub Issue #695. How can a big project like CraftCMS not have unit tests? It’s OK not having tests for all parts, but especially when dealing with numbers and money there is no excuse for not having unit tests.
Conclusion
In the long-term it hurts projects when developers only focus on new features instead of fixing existing issues. The main focus should always be the core of the system. The core has to be clean and stable and high-performance. Otherwise it will lead to a critical point where huge parts have to be rewritten, with a fresh approach.
CraftCMS has too many open problems, too many features which are not well design enough. Pixel & Tonic often use bad development practice that lead to immature code. This immaturity hurts the project and the trust of the clients in the long-term. Especially for a system like Craft Commerce, which deals with numbers and billing and invoices, by not spending time on quality.
At the company where I’m employed we already spend too much time fixing Pixel & Tonic’s bugs. We often have to find workarounds for CraftCMS and Craft Commerce to not loose the trust our clients are giving us. I’m willing to accept that CraftCMS and Craft Commerce can get better. It’s a long way to go.
We wanted both to cry and laugh at the same time about this code.
