Prosty CMS - Fat Free - Prośba o feedback

Question

Hej!
Od kilku dni siedziałem nad małym projektem, którym jakoby miał mi pomóc przyswoić język jakim jest php.
Stworzyłem prosty CMS, który posiada takie funkcje jak:

1. Dodawanie, edycja, usuwanie oraz kategoryzowanie wpisów
2. Listowanie wpisów wraz z paginacją
3. Dynamiczne MENU, które można stworzyć w panelu
4. Możliwość dodawania podstron w dynamiczny sposób za pomocą panelu.
5. Widgety - czyli elementy strony, które możemy w swobodny sposób wyświetlać, w dowolnym miejscu na stronie oraz edytować je w panelu.
6. Oczywiście prosty panel zarządzania oraz logowanie do niego.
7. Minimalistyczne wsparcie dla seo w postacie SEO Title oraz Description, które możemy zmieniać w panelu.
8. Dynamiczne ładowanie plików CSS oraz JS jeżeli tylko są w odpowiednim katalogu.
   webroot/css
   webroot/js
    

W planach mam rozwój tego projektu. Na początku oczywiście refaktoryzacja kodu na podstawie opinii, które uda mi się pozyskać od bardziej doświadczonych kolegów. 
Mam prośbę, nie zależy mi aktualnie na tym aby czytać co można dodać (z funkcjonalności), a jedynie chciałbym dowiedzieć się jakie błędy i niedociągnięcia popełniłem w samym kodzie.

W aplikacji wykorzystałem Fat Free FW głównie dla routingu.
Bazowy Controller oraz Model również wykorzystałem z gotowca. 

https://gitlab.com/raptoor2/simplecms/tree/master

Answer 1

Trochę przejrzałem, kilka sugestii ode mnie: 

• vendor nie powinien znaleźć się w repozytorium, od tego masz Composera.
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Functions.php - czegoś takiego bym unikał, ta klasa w sumie nie ma żadnego sensu istnienia (jest tylko po to, aby dostarczać jakieś losowe funkcje).
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Config.php konfigurację trzymałbym poza klasą, musi się dać ją łatwo zmieniać bez generowania zmian w repozytorium. Darowałbym sobie również takie komentarze, tym bardziej po polsku.
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Category.php#L17 - tak nie robimy, wszystkie parametry do zapytań należy bindować.
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Category.php#L55 nie prościej foreach skoro chcemy iterować po całej tablicy?
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Image.php#L55 obsługę błędów tego typu chyba jednak zrobiłbym na wyjątkach.
• Zastanowiłbym się nad trzymaniem komunikatów (treści błędów itp.) w samych klasach. Co np. gdy będziesz chciał dodać obsługę kolejnego języka?
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Search.php HTML gdzieś w logice modelu, to jest słabe.
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/User.php htmlspecialchars() na haśle? Czyli jak podam hasło np. z < to się nie zaloguje, bo to zamieni na jakąś encję. Poza tym md5 nie nadaje się do hashowania haseł.
• W kontrolerach jakoś dużo tego kodu, chyba starałbym się jeszcze więcej wydzielać chociażby do jakichś serwisów czy innych klas.
• Dziwne jest dla mnie, gdy kontroler nic nie zwraca. Nie wiem jak działa ten framework, którego użyłeś. Jak dla mnie po prostu ładniej i logiczniej to wygląda, gdy kontroler jeśli to możliwe daje jakąś odpowiedź, robi return.
• https://gitlab.com/raptoor2/simplecms/blob/master/src/Controllers/MainController.php#L46 - jak nie działa to nie wstawiamy do kodu, albo wstawiamy na inną gałąź i gdy funkcjonalność jest gotowa łączymy na główną :P
• https://gitlab.com/raptoor2/simplecms/tree/master/src/tmp/cache cache w repozytorium?
• https://gitlab.com/raptoor2/simplecms/tree/master/src/uploads pliki wrzucone przez użytkowników w repozytorium?
• Ogólnie co do całego kodu: wygląda jak taki lekko przestarzały, nie widzę np. typów argumentów czy typów zwracanych, które mamy przecież od PHP 7.
• Nieprzestrzegany PSR-2 - to nie jest żaden obowiązek, ale wg mnie ważny standard.

Comments

Wielkie dzięki za zainteresowanie, teraz moje pytania do odpowiedzi :)

 

1. https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Category.php#L17 - tak nie robimy, wszystkie parametry do zapytań należy bindować.
   Chodzi o funkcję "bind_param"? Jeżeli tak to jakie korzyści niesie jej używanie?
2. Zastanowiłbym się nad trzymaniem komunikatów (treści błędów itp.) w samych klasach. Co np. gdy będziesz chciał dodać obsługę kolejnego języka?
   Czyli powinien to zrobić na zasadzie jakiegoś pliku np XML, w którym trzymałbym treści komunikatów oprawione odpowiedzią nazwą? np NO_USER
3. https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/Search.php HTML gdzieś w logice modelu, to jest słabe.
   A jak powinienem to zrobić? Mam tutaj generowanie  formularza i w przyszłości chciałem dodać mozliwość budowania tego formularza w panelu stąd umieszczone tagi w logice.
4. https://gitlab.com/raptoor2/simplecms/blob/master/src/Models/User.phphtmlspecialchars() na haśle? Czyli jak podam hasło np. z < to się nie zaloguje, bo to zamieni na jakąś encję. Poza tym md5 nie nadaje się do hashowania haseł.
   Czyli powinienem używać sha1 + salt zamiast tego starego md5?

---

1. Zapytanie jest wysyłane osobno i parametry osobno. Dzięki temu nie ma żadnej możliwości wykonania ataku sql injection.

2. Na przykład. Może to być równie dobrze zwykła tablica w PHP zwracająca klucz - tłumaczenie, coś w stylu: 'user' => 'Użytkownik'. Do tego można to jakoś podzielić, dodać obsługę wielu języków i wydaje się to wtedy całkiem sensowne.

3. Ja bym chyba i tak przerzucił to do osobnego pliku widoku, który by się renderował i zwracał do zmiennej. Dalej w razie potrzeby możesz sobie go podstawić gdzieś do panelu.

4. sha1 nie jest nic lepsze, bcrypt albo Argon2 (password_hash + password_verify).