Code standards, etc.

[merumelu]

Okay, now for something important :D

At the moment, the code uses a mixture of different styles. For example, while most of the files use two spaces for indentation, some use four and others go a different way altogether and use tabs.

Another example: in templates there are a few occasions where alternate sytax is used for control structures (like if (foo): ... endif;) which do not appear in the majority of the code.

It might be good to decide on some standards while the codebase is still smallish, so changing it doesn't become such a behemoth as godotengine/godot#3916 :stuck_out_tongue:

[merumelu]

Btw, PSR-1 and PSR-2 are pretty much the standard in PHP nowadays.

[bojidar-bg]

Ok, here are some things that are (I think) already most-used:

• 2 spaces (never 4 or tabs), except in generated files (composer.json and bower.json)
• In templates: Always <?php if(stuff) { ?> <?php } ?> (full tags and braces)
• About utility functions (we should change those, since its pretty much a mess, unfortunatelly):
  • The first parameter should be the container (if the utility function is not part of a class)
  • The second should be $error or $currentStatus, and the function should immediately return if it's true
  • The third should be the response passed by reference (&$response), and should not be modified if there were no errors
  • If there was an error, the utility function must return true, and change the response to include the message
  • Otherwise, return false

All of this allows us to call utility functions as:

  $error = $this->utils->error_reponse_if_missing_or_not_string(false, $response, $body, 'username');
  $error = $this->utils->error_reponse_if_missing_or_not_string($error, $response, $body, 'email');
  $error = $this->utils->error_reponse_if_missing_or_not_string($error, $response, $body, 'password');
  if($error) return $response;

[TomWor]

I'd strongly suggest going with PSR-1 & PSR-2. https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md

Some important points:

PSR-2

• Code MUST use 4 spaces for indenting, not tabs
• Opening braces for classes MUST go on the next line, and closing braces MUST go on the next line after the body.
• Opening braces for methods MUST go on the next line, and closing braces MUST go on the next line after the body.

PSR-1

• Class names MUST be declared in StudlyCaps.
• Method names MUST be declared in camelCase.

So it should be e.g.

$error = $this->utils->errorReponseIfMissingOrNotString(false, $response, $body, 'username');

I'd also recommend using editorconfig.org to easily set editor preferences to those styles. Added a pull request for a basic editorconfig file #87.

[TomWor]

It might be related, so I'm posting this here: After browsing the code a bit I noticed a couple of odd design decisions.

• $this->utils->ensure_logged_in() should definitely be handled by authentication middleware, not a helper class
• Passing a 'status' from one error checking method to the other $this->utils->error_*() means executing an error check, even after the intended action already triggered an error, means unnecessary error checks and possible bugs down the line; should hard-exit in some way; again: middleware should be utilized to handle most of these methods
• Asset related routes should have a clearer distinction between moderater and owner routes, see Route Groups

• The database handling stands on really slippery ground IMO; I don't necessarily recommend using Doctrine or another heavy weight ORM solution, but an object based wrapper would work wonders:

  $asset = new \GodotAssetLibrary\Asset();
  $asset->title = 'PoolManager';
  $asset->save();

• Some utility helpers have a typo: 'reponse' => 'response'
• The routes functions are too big and should be extracted to objects handling the domain logic, see e.g. 'get_asset'
• Routes should follow RESTful naming standards more closely and could be cleaned up in general
• Frontend assets should not be linked to directly from the bower directory, there should be a build process so further custom frontend code can be minimized with the third party libraries; I'd also recommend switching from Bower to NPM, bower became a bit old-fashioned lately, people switched to NPM

[bojidar-bg]

I agree for all except the last point -- I'm not switching away from any technology since people found it "uncool" (also, bower has pretty nice decentralization I personally like). If we would switch away from uncool technologies, we are going to have to run assetlib on a Go/Rust server (which is obviously not on tuxfamily..).

[TomWor]

@bojidar-bg I completely get this - I also stay with some older technologies, because I'm more productive in them; and a project maintainer / main dev should always feel comfortable with the tech stack they are using. I guess I made a bad argument for chosing NPM over bower. Let's see if I might convince you further down the road for a better reason. ;)

[TomWor]

btw.: What about using a template engine? (Smarty/Twig) I know all the arguments around it (and been through the holy wars) - but I'm generally in favor of template engines, because they add a distinct interface between logic and presentation and make writing view helpers very modular.

(I'm also more of a frontend guy and thought I might be of use there a bit down the road)

[merumelu]

There is a template engine used, PHPView. However, it's pretty bare-bones.

[merumelu]

If we want to keep templates as PHP instead of some custom templating language, there's also Plates.

[bojidar-bg]

Ok... seems like we are deploying beta soon, so, we should start thinking about this issue, before 1.0 is started.

In other words, I would like if everyone agrees on some plan of refactoring action, and then we (or I) might do it. :smiley: (not implying that we aren't agreeing on something in any way)

[merumelu]

So does the merging of #87 mean ok to four spaces or PSR1/2 as a whole? If it's the latter, let's do that first, might be enough to just run everything through php-cs-fixer. :smile:

If we are going to replace PHP-View with something else, I'd vote for Twig. It has nice stuff like template inheritance, and unlike templates written in vanilla PHP, there's also autoescaping available as well as specific escaping strategies for JavaScript or HTML attributes which lessens the chance for accidental XSS quite a bit. Smarty might have this as well but I've never used it.

For auth stuff, I'd suggest adding a middleware which adds a user to $request using $requst->addAttribute(), which can then be checked in route middleware and is also accessible in routes themself if needed. Maybe add a "guest" level, so is_logged_in + has_level_higher_than(x) can be combined.

[akien-mga]

Regarding applying PSR1/2, please do it in two commits:

• first change the indentation everywhere
• then apply php-cs-fixer or similar

[merumelu]

Hm, PSR-1 states:

Code written for PHP 5.3 and after MUST use formal namespaces.

What namespace should we use? Godot\AssetLibrary?

[bojidar-bg]

@merumelu sounds good... :smile:

[TomWor]

I'd volunteer for integrating the template engine if you like. I'm a bit more in favor of Smarty, because I've used it extensively in the past, it's said to be faster, and extending it with custom functions is more straightforward IMO.

Just check the docs: http://twig.sensiolabs.org/doc/advanced.html#creating-an-extension vs. http://www.smarty.net/docs/en/plugins.tpl

I know and recognize that Twig has (for some reason unknown to me) more momentum right now, so if you like to use that, I can do that.

Plates looks also nice I gotta say, but never used it.

I also have a bit of experience with Slim middleware, so if I should look into that, just ask.

[bojidar-bg]

Ok, I'll try to summarise everything decided on, plus some things that are under consideration:

• PSR-1 and PSR-2 apply.
• Utility classes are well-refactored somehow.
• We start using route groups and middlewares to handle admin/moderator/user/anyone permission levels
  • Probably utility classes like the token class would be obsoleted by this.
• We use the Godot\AssetLibrary namespace for all classes
• We start considering npm instead of bower, though we might just use git submodules if we want
• We have to decide on a templating library to use:
  • Plates & PHPView - Both use php syntax. Unfortunately, some configurations of php don't support the <?= syntax, and it might be "rude" to ask developers to change configs just so they can run a local version. So, maybe yes, maybe no, even though they should be very fast in general.
  • Smarty - Advertised to be fast, syntax is pretty clean, even if using { might collide with normal text at times (though that's probably not a big issue). Doesn't allow php in templates directly, which is probably nice.
  • Twig - Nearly same-looking as Smarty, though it uses {{. Also, doesn't allow for php in templates. Probably, being a bit slower than Smarty is the only problem with it.
  • Blade - A bit hard to pick a "fork" without Laravel, since there are a more than a few. Uses {{ like Twig and Smarty, also used in a pretty sweet PHP framework (which I used once). Uses a pretty strange syntax for functions though (@..), so it should be considered carefully. This link might be useful for browsing through the options for provider.
• The DB should be refactored to use a more OOP approach, instead of "raw" SQL calls. Might need a bit more discussion, who knows.
• We have to pick a minimum supported PHP version. (see #95). Current hosting provider gives 5.6.27-0 with some zend cache.

[TomWor]

The PHP short tag syntax <?= is not related to PHP being old, it's been around for awhile, but it might be disabled on the host as you say - have you tested it? And yes, without it, it's really no fun to use a PHP based template language and would be an argument for one of the other solutions.

[bojidar-bg]

@TomWor Ups, my bad, it is my configuration that's wrong, not theirs :laughing: But, that being said, php-based templating might lead to putting too much things in the templates, which shouldn't be there (as mentioned by the Smarty site).

[vnen]

I remember having performance issues with Twig in the past, though it was a distant past and the problems went away once I rewrote my code to better use the caching it offers. Syntax-wise it is my favorite.

[TomWor]

As I'm said I'm rather agnostic about it, but I prefer Smarty syntax. Small example:

Twig

{% for color in myColors %}
    <li>{{ color }}</li>
{% endfor %}

Smarty

{foreach $myColors as $color}
    <li>{$color}</li>
{/foreach}

And writing plugins, filters, etc. is more straightforward IMO, linked it above. Anyway, let's just decide and move on, the template migration would be a good issue to move out of this thread and just implement it, it's very straightforward.

[bojidar-bg]

@TomWor Ok, I'm for Smarty in this case, since it is really troublesome to write all those % signs (though I dislike the ancient look of their doc :laughing: )

[TomWor]

@bojidar-bg Yeah, those % signs make my eyes bleed and I don't like them for readability. As for Smarty Docs: Yes, we are definitely not in cool-town here, but to their defense: The documentation is really good and clear. :)

[TomWor]

I might have a bit of time next week to work on the template engine integration. Can I proceed on this and start creating a template branch on my fork?

[bojidar-bg]

Well, would be nice if @merumelu's PSR PR is merged first, but it has some conflicts... So, I guess, go for it, then if some merge conflicts occur, we would fix them by hand :smiley:

[bojidar-bg]

@TomWor How is it the template engine integration going along? I would be glad to help somehow...

[bojidar-bg]

Just an update:

• @Calinou will be migrating the assetlib frontend to Twig and Bulma, see #135, #134
• Alongside that, we will be migrating to Yarn/NPM instead of Bower, since Bower is dying away nowadays. (#136)
• Routes are going into controllers with middleware (working on it now)
• Database is going to be handled through some ORM, or if none fits what we need, just an active record implementation will do it. This is still to be decided.

[cmfcmf]

That is great news! I would be willing to integrate the Doctrine ORM, if you like.

[bojidar-bg]

@cmfcmf Well, I checked various ORMs available, and I am not sure about using Doctrine (feel free to correct me on any of the points, as I haven't used PHP much).

Here are the various points I think are important to have (in no particular order):

1. Clean and well-written documentation. A few ORMs have hard to understand docs, and will thus be harder to maintain in the long run (since maintainers change).
2. Easy to develop without any tools installed, though it might generate cache files at runtime. The rationale here is that it is a bit hard to install composer or other tools on our current hosting provider, as well as maintainability (since contributors won't have to install the tools or wade through configs).
3. Supports MySQL, and ideally also supports other databases. Otherwise, it might be harder to migrate away from MySQL if needed one day.
4. Written with some performance in mind. Since you never know when a bottleneck will be hit.
5. (bonus) Does as little destructive things to the database as possible. This usually means less black magic and potentially easier migrations.
6. (bonus) Supports migrations out of the box. Not required, as there are libraries for migrations.

Taking a list of ORMs from awesome-php:

• Atlas.Orm - Docs provided inspiration for point 1, even though it might be cool by itself.
• Aura.Sql + Aura.SqlQuery - Query builder + PDO utils. Not exactly an ORM, but might fit the needs.
• Cake ORM - Not that bad, though it seems a bit overly comprehensive.
• Doctrine - Configuration through annotations is a bit ugly. Needs to update schema after updating annotations, see 2.
• Eloquent - Everything is configured in PHP. Seems like it might fit the points, though I remember it being a bit of a nightmare sometimes.
• LazyRecord/maghead - Uses configuration YMLs, fails 2. Will use if being fast is a requirement.
• Pomm - PostgreSQL-only, see 3.
• Propel - Beautiful XML configuration, but fails 2.
• RedBean - Configurationless, just goes forward and creates the DB when you use it. While not a bad idea by itself, it likely fails 4, as well as being too much magic.
• Spot2 - Everything is configured in PHP; simple concept. Seems like it might fit the requirements.

[cmfcmf]

You are right, I should've looked at other options before suggesting Doctrine (I suggested it because it is the only ORM I have used so far). Thank you for your comprehensive list of requirements and alternatives! I've looked into the different options myself now. Here are my two cents in addition to your own evaluation:

👎

• Atlas.Orm: Hard to read docs, very small project
• Aura.Sql + Aura.SqlQuery: Not a real ORM
• LazyRecord/maghead: Last commit almost a year ago, current version says "4.0.x IS CURRENTLY UNDER HEAVY DEVELOPMENT, API IS NOT STABLE"
• RedBean: Too much magic
• Cake ORM: Thightly integrated with CakePHP, i.e. many dependencies: https://github.com/cakephp/orm/blob/a65417cd66c4a64886184ec04eadb5d92d451af7/composer.json#L26-L33
• Propel: Propel 1 unmaintained, Propel 2 still in alpha, Propel 3 in development

👍

• Doctrine: Does indeed seem overly complex for this project, however is proven to work and by far the most popular one (based on GitHub stars)
• Eloquent: Looks really nice and simple, uses ActiveRecord pattern
• Spot2: Looks really nice and simple

To me it seems like Doctrine, Spot2 or Eloquent may be the way to go.

[Calinou]

I would personally go for Doctrine as I'm most familiar with that ORM.

[rluders]

Great! Let me point here something.

Actually the Godot's website is using OctoberCMS that is developed over the Laravel Framework. That is amazing. In fact, the big discussion, IMHO, should be if the Asset's Library desires to be close to the technology adopt on the website, allowing both to get easily connected.

So, my suggestion is to migrate the application to Laravel, using Eloquent and Blade/Twig (I really don't care about the template engine at this point) and Bulma CSS.

I started something on my fork, just for testing. Can be cloned from here: https://github.com/rluders/godot-asset-library/tree/feature/laravel

[bojidar-bg]

@rluders Sorry, I think I will have to stop your enthusiasm a bit.

My main concern, as discussed on other channels, is that there is little point in switching the whole codebase unless there are unsolveable problems with the current one. This is commonly known as "don't fix what's not broken."

About the big discussion of whether the Asset Library should become an OctoberCMS plugin, we have never planned for such an inclusion. Actually, part of the original plan was to provide code which would be used to create other asset libraries easily.

A final concern is that the changes when switching to another framework are huge, and learning to maintain the new code would take some time.

[TomWor]

I think a change to Laravel would make a lot of sense.

Sadly I never came around to contribute much to the project since my first involvement 2 years ago (!), and looking at the commit graph, there was really not much happening here by other actors as well. (no offense) Part of the reason I think might actually be the non-inclusive nature of the codebase, which is a bit of a hack based on the Slim Framework without any regards to current PHP code styleguides and best practices. What Laravel would provide is a stringent framework philosophy that a lot more developers are already familiar with and would provide a codebase that could be more easily picked up and worked on in peoples spare time. Please don't discard the idea for a framework switch and effective rewrite so easily. I think the whole asset library could be rewritten by someone with Laravel experience in about a day or two, that's how efficient the Laravel workflow is (I'm getting into it right now too, work-related). Also want to point out @rluders didn't propose making the asset library an October CMS plugin, just aligning the technologies with another, which would be another slam-dunk strategy-wise in my books. Just my 2 cents.

[rluders]

@bojidar-bg so, I totally disagree with the “don’t fix what’s not broken” principle. And I’ll explain the main reason for that in one second.

@TomWor you came with a lot of points that is the reason to move to Laravel. And when you said that non-inclusive nature of the code is reducing the number of contribution, I can’t agree more. And this was one of the main reasons for me to propose the Laravel.

My first idea was exactly to help the project has it is right now, but It was so confusing and difficult to find out where to start. And I consider myself an experienced PHP Developer and Software Engineer (you can check my LinkedIn if you want to). And at the first moment I was just reading the code and trying to find something to improve, and my second thought was: OK, maybe the way that I can help is to make it an familiar codebase and some design principles, that could make it grows SOLID.

The code right now is pretty small, and most part of then could be easily moved out to Laravel. I’m working with the rewrite right now, on my fork, and I’ll probably work hard this weekend to see how much of the concepts can I delivery.

I know, Slim is great, in fact, there’s a lot of nice frameworks that could be used. But, my big concert about using micro-frameworks is that you need to spend too much time focus in “designing principles” that an Laravel already provides, you don’t need to research libraries, see how they work together, design your abstracts, your folder structure, and more. You already have the basic job done, you could just keep the focus in your code business.

I know that we already have some work going out, to using some ORM and also moving the template engine to Twig. And I feel like we don’t need to discard this jobs, ‘cause we could use Twig instead of Blade with Laravel, and also using Repository Pattern to embrace the non-eloquent ORM for know, or even move it to Eloquent will be not be THAT painful.

And yes, @TomWor, align the technologies will give us a lot of benefits, ‘cause we could have people working on the website that will also be able to help on the asset store. So, instead having two teams, we will have ONE big and integrated team. So we could easily integrate system and contributors.

[rluders]

So... Here... As I said before, I'm working in an complete (?) rewrite of the asset store using Laravel. You can check an preview here: http://godot.luders.com.br/

[menip]

@rluders Your ui seems clean. What is status of project?

[rluders]

Hi, @menip. I just didn't have time to work on it right now (too many things happening). I'll get it back after August. Any help to work on it will be appreciated. Feel free to ping me on IRC (at freenode: rluders) or even on Godot official Discord (rluders#5401).