Typo in switch case block in file modules/Emails/EmailUIAjax.php

[muratyaman]

case "doAssignmentDelete"; it must be: case "doAssignmentDelete":

Quick note: this file is another perfect example of how NOT to write code :(

• todo: encapsulate logically related code into separate classes -- e.g. use (API) controller class(es) -- contact management API -- folder management API (create, rename, delete, etc.) -- email management API (read, compose drafts, send, relate to other entities etc.) -- signature management API -- attachment management API
• avoid: inconsistent behaviour/output: in some cases JSON, in others non-JSON output e.g. "NOOP"
• avoid: very long switch case block
• avoid: cases without break statements are generally confusing
• avoid: too many and too long if blocks
• avoid: relying on global variables e.g. $_REQUEST
• avoid: reading array items without checking index/key
• avoid: changing PHP settings: e.g. changing the threshold for execution time. -- the users do not wait for 5 minutes (300 seconds) in front of a browser, they just keep clicking every few seconds or so, or refresh the current page, navigate away to another page, etc. If PHP is setup to ignore user action, then it will continue processing a request until it finishes; and it means that there could be parallel threads for the same action by the same user and the code has to be re-architected to handle such scenarios. -- if there could be tasks which may take unknown amount of time (e.g. reading 500 unseen emails (with/without attachments) from inbox - see cases for checkEmail and checkEmail2), they should be performed by background workers connected to a proper queue service.

[pgorod]

@muratyaman thanks for the report and the "quick note" on code quality.

There is so much to be done in this project, we inherited thousands of lines of code and some of it is really needing a rewrite.

Can you help with some of it? From what I normally see here, just raising issues with recommendations doesn't get much changed, because there simply isn't enough time for the few developers that work on this to fix everything.

If you could write a PR fixing just two or three of the issues you list, even the easiest ones, that would be really helpful. We need more engaged members of the community, especially if they are sensitive to best practices and code quality issues like you are.

[chris001]

(facepalm) Thank you @muratyaman for finding an inexcusable, ridiculous syntax error crash bug, yet another perfect example of why @salesagility really ought to immediately enable PHP-CS-FIXER, PHP_CS, Scrutinizer-CI, AND merge the PRs mentioned here #3688 - to find ALL of these bugs automatically and quickly, without effort.

There's really no point to making any further PRs whatsoever, until these very basic essential code quality and reliability PRs mentioned in #3688 are merged into master, hotfix and develop, so that these code checkers actually run on the code! Check the code and find problems for all to see plainly. For the purposes of fix them more effectively. It's time for this legacy PHP app to join the modern era of automatic continuous integration.