View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 1568 | Composr | core | public | 2014-02-14 01:53 | 2014-10-26 20:01 |
| Reporter | Chris Graham | Assigned To | Chris Graham | ||
| Priority | normal | Severity | feature | ||
| Status | resolved | Resolution | fixed | ||
| Summary | 1568: Implement PSR-2 | ||||
| Description | I've taken a look at PSR-1 and PSR-2, and some things we can do and some we cannot. There's no way we'll reasonably be able to claim of "supporting it as a standard" without burdening the project, but we can try and meet some of the conventions. Our Code Quality Checker tool would need updating as well as Composr. The v10 editorconfig file would too. Probably some automated tests would want adding to maintain some of these (e.g. "All PHP files MUST end with a single blank line."). The coding standards checklist should also be updated. Of what we don't currently do, I have split it into two classifications... Can do ====== >> Code MUST use 4 spaces for indenting, not tabs. We can do this. I don't totally like it, because pressing the right arrow in an editor is not going to work so well. But, there'll be shortcuts and they can be learned. I recognise that 4 spaces is more common and it's better to therefore go with that. We currently have a tab size of 3, which comes from C but the popularity seems to have dropped significantly. If we do it, we should do it for Javascript also. This is fine as we do automatically minify Javascript. I don't think I want it for CSS because they only have one indentation level anyway. Plus CSS is commonly edited within Composr, and people wouldn't be more patient about the clumsinesses of spaces in that. >> There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less. >> Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line. >> When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them. I'm fine with this as long as lines are not split up in such a way as the cuts look arbitrary. It should align neatly with parse structure/grammar. There's no way we are doing this manually, so a tool would have to be used. >> There are many elements of style and practice intentionally omitted by this guide... Operators and assignment We may as well add spaces around these, as it seems to be what most other people are doing. >> Visibility MUST be declared on all properties. >> The var keyword MUST NOT be used to declare a property. >> Property names SHOULD NOT be prefixed with a single underscore to indicate protected or private visibility. >> When present, the abstract and final declarations MUST precede the visibility declaration. >> When present, the static declaration MUST come after the visibility declaration. That's fine. >> Opening braces for control structures MUST go on the same line. That's fine. >> All PHP files MUST end with a single blank line. That's fine. >> null MUST be in lower case. That's fine. >> In the argument list, there MUST be one space after each comma. >> A for statement looks like the following [spaces after each semicolon] >> A foreach statement looks like the following [spaces around "=>"] That's fine. >> else and elseif are on the same line as the closing brace from the earlier body >> a do while statement looks like the following [while on same line as closing brace]. >> a try catch block looks like the following [catch on same line as closing brace from the earlier body]. That's fine. No can do ========= >> A file SHOULD declare new symbols (classes, functions, constants, etc.) and cause no other side effects, or it SHOULD execute logic with side effects, but SHOULD NOT do both. There are very few cases where we technically fail to meet this, but in spirit we do not. For example sources/lang.php will initialise the language system, which means loading default files etc. sources/users.php will initiate the users system. Splitting up these files would make no sense for a number of reasons: 1) Over-complex 2) Too many files 3) Performance slow down (extra seeks / cache buckets) 4) Many functions require initialisation of state before they may run (we could try and check state and auto-initialise upon calling, but that would definitely be a measurable performance hit for something with heavy use, e.g. do_lang) It's easy to think that small overheads don't hurt performance, but actually adding extra checks or dereferences to critical functions can result in 10's of thousands of additional operations per page view, because Composr is designed to pull together a lot of very dynamic content from a wide range of sources. We could make the tail-end of our code (e.g. APIs for particular content types) much more modular, but it'd be pointless - as it is the core code that is most commonly coded against. It's better to keep it consistent too. >> Namespaces and classes MUST follow PSR-0. This means each class is in a file by itself, and is in a namespace of at least one level: a top-level vendor name. Composr's code is intentionally very terse, and we make it very easy to call things up. We're not a framework serving a pool of diversely contributed classes, we're aiming for maximum performance on every measurement, and adding in extra abstraction definitely hurts that in a few ways (e.g. memory, PHP parse time, PHP parse tree size [opcodes], call tree complexity, disk reads, ...). I am very aware that the way we do our code is highly controversial -- but performance in terms of programmer efficiency (*), and in terms of server performance, and general project size (number of files) are huge factors in a system like Composr. (*) Composr is probably the most feature rich CMS out there, we try and support everything common, and that's a huge amount of coding effort, maintenance effort, and cognitive load to understand - so we just can't afford to double code length to add extra abstractions that some kinds of programmers like to make. Additionally, we support PHP 5.1 still even with v10, so no namespaces. While we could review this (PHP 5.3 is probably fairly safe), I know there will be a non-trivial proportion of users still on 5.1 out there, because hosts favour legacy over what platform developers like (they should not, as old security holes may be there, but they do). I'm also not sure why we would want namespaces in Composr classes - they are tightly bound together (again controversial, but intentional). They make perfect sense when using other people's libraries though, for custom long-tail functionality or particular complex business reporting implementations. >> Class names MUST be declared in StudlyCaps. Class names often mirror other identifier names. For example, block names represent the actual name of the block, and blocks use underscores. I recognise that most people use StudlyCaps, but the way we simplify things by unifying/automating identifier linking through the system means we can't have multiple naming schemes for identifiers. Automatic conversion is unnecessary complexity. >> Code written for 5.2.x and before SHOULD use the pseudo-namespacing convention of Vendor_ prefixes on class names. We're not a framework primarily for programmers. Our main selling point is the integrated features that already exist. So the general workflow of Composr site development is not putting together a custom arrangement of classes. Therefore vendor specification would be unnecessary complexity. Other PSRs ========== I looked at PSR-0 (autoloading) We do not use autoloading, as library APIs are mostly global functions, not classes. I am very aware it's highly controversial, but it is intentional to make it easier to code, and for performance (less dereferencing, not needing to create and hold objects, not making it hard for us to split stuff up at a later date). If we autoloaded classes it would be confusingly inconsistent, given the same doesn't work for functions -- better we have a consistent style of calling up dependent code. I believe at some point function autoloading was being discussed in the PHP community, but I don't think it's happening any time soon. I looked at PSR-5 (phpdoc): We have a separate syntax for declaring possibilities to return false or NULL. This is intentional, as we implement them as type mutators. This is consistent with what database software does, what our own database type system is doing (implemented on top of normal DB types), and allows us to do type-checking in our custom version of PHP. It is also consistent with how HHVM Hack declares types. | ||||
| Tags | No tags attached. | ||||
| Attach Tags | |||||
| Time estimation (hours) | 10 | ||||
| Sponsorship open | |||||
