opencart / opencart

A free shopping cart system. OpenCart is an open source PHP-based online e-commerce solution.
https://www.opencart.com/
Other
7.46k stars 4.84k forks source link

1.5.6 coding standard gone? #799

Closed JAY6390 closed 11 years ago

JAY6390 commented 11 years ago

While I realise it's not a major issue, it's still a pain for us developers using vQmods on the product editing form when things like <?php echo $footer; ?> in the admin/view/template/catalog/product_form.tpl now has //--><?php echo $footer; ?>

which means every "before" vQmod using this is going to break. Can this be amended and the 1.5.6 download updated ASAP so that we don't screw up loads of mods because of one cock up

ghost commented 11 years ago

It would be nice if we got some official standards for coding style and folder structure and enforced them. They've started slipping and I'd hate for things to end up a mess like ZenCart.

Also, OpenCart desperately needs release candidates. We've been hearing that 2.0.0 was the next version for months now then all of a sudden 1.5.6 pops up out of nowhere and leaves developers scrambling to check if their mods are compatible (I'm not sure if vQmod even works for it).

JAY6390 commented 11 years ago

@opencarthelp There are gaps that I can see in terms of vQmod. Unfortunately these won't be getting fixed right away. To be honest, having things like cba_cron.php will mean they won't be vQmod enabled. These could so easily be written into a controller. It's just laziness

jamesallsup commented 11 years ago

Extra space has been added. It was not present in the dev IDEs, seems to be an issue at GitHub when you download the zip or clone.

FYI, creating cron jobs using WGET command is insecure and poor practice. While I agree that a controller will be MUCH better, you can't place that outside the web directory. The cron file should be placed above web dir in a separate folder - so its not laziness, consideration for it has already been thought of (FYI I was in favor of a controller due to the script nature).

Personally I think that OC should have a single cron file anyway that mods can hook into, but that's another discussion.

If you have any suggestions then put them forward, please don't just criticize work!

JAY6390 commented 11 years ago

Simply adding a random hash as a key would make the cron more than secure across wget. As it stands there's nothing and no way for people to know nor is there any security on the file or documentation that it needs to be moved. This means every OC 156 is going to have this script with no security check and nothing in place in the install etc to tell the installer to move it/remove it. As for putting ideas forwards/criticising its not really possible when its not even known the changes are getting integrated until they are already in place. This is why I've voiced them afterwards

jamesallsup commented 11 years ago

Ok will get this looked at in the morning, like the idea of the hash. Probably something like an md5/sha1 of a string entered by the user and given the url after the save.

Will create a new issue for this.

JAY6390 commented 11 years ago

Would say either is ideal, could even get it auto populated when the install occurs (like the security key does by default). Would recommend keeping it separate to that though