Alittle feedback from a new user and a developer

[durangod]

Hi, Well first of all relax i like the script. I like the admin config layout, the userside, and it scores high marks for me in support which is excellent.  So i will continue to use it. 

Now with that, there are a few things that i found to be old school, and needs work. 

1. First and foremost, get the sql queries out of the core files. All of those need to be in a in a protected class and then a request class needs to be created to get the data from the protected class.  Having queries in the core files is 2008 style coding and people just dont do that anymore and for good reason.   

The db requested data should flow this way.  File request -> public class method and that then calls the protected class query and then the data returns in the same path back to the file.  This provids much better db query protection than having queries in the core files. 

Core file request ->  request public class -> protected class with queries  and returns the same direction.   In other words nothing has direct from core files to the db queries, it has to go through a request class and that request class is the only thing that has direct access to the db queries.

Its also so easy to fix, just change the db query in the file to a request function, then after it  it goes to the protected method, on the way back way back just return the data to the file, bingo same as it was but now you have a more protected db.

2. Too many orphaned elements with no class or id.  Almost everything needs to have some sort of class or id assigned to it, even <p>'s  because one never knows when someone may need to isolate things right down to the text. 

3. Camelcase file names, IMO this is a bad habbit and something from the past.  Use a underscore for name separation.

4. Way too reliant on js. This causes undesired conflicts. 

5. Not enough module coders and not enough modules available/updated to run on the current version. 

All in all yes room for improvement but i think you have a really good thing going here and i will certainly continue using the script. 

Nice work, look forward to the improvements.  :0

[Kindred]

1 is not as simple as you make it out to be... but 3.0 is a big change to how things are coded

Not sure I agree with 2...  but that's easy to solve  -- write your own theme and add all of that. 

3 is personal preference...

5 well, we really have no control over that

[durangod]

Agreed and cant wait to see 3.0  but number 3 is more than just a preference. 

Have you ever had two file names exactly the same name but one is camelcase and one is not.  We have to make sure that the file names are designed so that someone cannot spoof a file.

on my server php sees these two files as different

Admin.php
admin.php

however on some server configs php can see those two filenames as exactly the same. This is why camelcase names is a bad idea.  It should always be  admin.php or file_name.php  always lower case for more stablity. 

[Aleksi "Lex" Kilpinen]

Linux is generally case sensitive, you can have both admin and Admin, but Windows is generally case insensitive and would indeed treat them as the same file - Which would mean that they would also be physically incapable of existing at the same time. So, I'm not sure I understand the problem. As long as there is consistency and logic to the naming, it's fine.

[durangod]

The traditonal usage more times than not across the board is name_name.ext with function names being functionName() or className being standards as well.  Sure php can handle camel case however why not just avoid any possible caveats down the road by sticking to a wider accepted practice. 

[live627]

3. Camelcase file names, IMO this is a bad habbit and something from the past.  Use a underscore for name separation.

but the user never requests those files, and the autoloading standard that is PSR-4 is at odds with your statement.

4. Way too reliant on js. This causes undesired conflicts. 

I dare you to turn off JS and load any page of SMF. Now try the same thing with Twitter and Youtube. Do they still load? Which site uses too much JavaScript and is therefore too brittle?

[durangod]

The auto loader has caused more issues over time than i can count.  Yes i understand and agree with what you are saying. However, what i am talking about is common standard practices that the majority follows, and that is lowercase with underscore and camelcase functions and classes, not filenames. 

I dare you to turn off JS and load any page of SMF. Now try the same thing with Twitter and Youtube. Do they still load? Which site uses too much JavaScript and is therefore too brittle?

I think you took this statement on a much grander scale that i meant it.  I never said that any site would load without js. Even my own project would not load without js.  Well mainly because i require it and if js is not enabled i tell them to turn it back on and i remove all the form buttons if they have it off.

Anyway my intention was to say that SMF relies on js too much. Too much js means that it uses js as the automatic go to when there are other ways of processing the same userside content. All this probably means is that the original author was very knowlegable in js and so that was their go to for much of the userside functionality.  That is not a negative and neigher was my original statement.  It was an observation from someone that has had to learn other ways becuase i honestly suck at js. I can do it but since i am not that affluent in the language i have had to learn other ways.

When i started my project one of the top 5 rules was to limit js as much as a can.  This helps prevent future conflicts. Especially when it can be done another way, why go js when its not necessary.  That is where the core of my statement was from, my own experience. 

[Tyrsson]

The underscore in class names is literally a hold over from the days of PSR-0

https://www.php-fig.org/psr/psr-0/

Which was deprecated as of 2014-10-21. The same deprecation notice on the page I linked to suggest PSR-4 as the replacement. What does PSR-4 suggest? If you follow PSR-4 I promise you, your autoloading will stop giving you problems.

My experience with a lot of libs is that you may have odd naming but it *usually* stops where the filesystem is mapped to the namespace. It's so much simpler to just follow the standard. PSR-4 is THE standard today.

As for the suggestions on the DB. What is needed is a true OO DBAL. I agree it needs to happen but I disagree with your suggestions on the implementation details. Also, I'm not even sure what you mean by a "protected class".

I prefer DBAL that implements the Tablegateway Pattern and provides a full set of predicate objects. I do not care for ORM. Most of the time it's overkill. If the DBAL supports mapping relationships between tables, that's a bonus, but if not I can just build it myself on top of the core classes. Generally I tend to wrap the DBAL classes in a Repository/Entity pattern which in most cases will ease unit testing. Not to mention you can have a trait express the query layer through predicate object usage and implement composition over inheritance which makes it exponentially more flexible.

Just my two cents.

[Edit to add]
There is a lot of discussions and planning going on currently in regards to many things for 3.0 so please be sure to keep an eye on the updates to the "development updates" that will be made from time to time The code is on github and anyone can contribute. If you have the skills we can use all the help we can get. Please read the relevant information on how to contribute etc before opening a PR

[durangod]

As for the suggestions on the DB. What is needed is a true OO DBAL. I agree it needs to happen but I disagree with your suggestions on the implementation details. Also, I'm not even sure what you mean by a "protected class".

Yes i agree i totally misspoke, i meant to type protected method. ooops i will correct that if its not too late..

:]

[live627]

limit js as much as a can.  This helps prevent future conflicts

Can you expound upon this? I don't understand

[Tyrsson]

Well, I should have said. I prefer a DBAL that supports the implementation of the Tablegateway pattern... Six of one, half dozen of the other... Semantics really, but one is more correct than the other

[durangod]

limit js as much as a can.  This helps prevent future conflicts

Can you expound upon this? I don't understand

Sure, it comes from my experience with open source scripts (trouble shooting modules/plugins and coding them myself even) that allow user created modules/plugins. 

My experience has been that on those scripts if the script itself does not have a well regulated approval process for modules/plugins then i have found that js conflicts happen at a much higher rate.  This is partly due to the main script relying too much on js in the first place.  However, it is just as much the fault of the module/plugin developer either not testing effectively and/or trying to do something with js that should not be done with js.  Especially once a conflict is discovered. 

So this has developed my thought that it is much better to not rely on js to do something that can otherwise be done some other way without adding a bunch of code, as i also think about footprint as well as server time.

[Tyrsson]

It would help if I could see the application that you have built that allows for user created plugins.

[Kindred]

But, you see,  we ***DO*** have a well regulated mod submission and approval process!!

[durangod]

It would help if I could see the application that you have built that allows for user created plugins.

Yes i agree but my project is still in developement. My point was is that i have past experience with same and i was making a general statement.  I used to code mods for WHMCS, Oxwall (both as a dev and as a approval team member), and several other sites.  That is why i said i had experience in the issues that over reliance on js causes. It had nothing to do with my current project.   

But, you see,  we ***DO*** have a well regulated mod submission and approval process!!

That is awesome, that will help so much in the long term. I am very glad to hear that. I was not saying you did not, it was a general statement, sorry if it sounded as i did.  :]

Anyway, i am glad we all got to share points of view, that is what its all about and i respect everyones point of view regardless.   

I think this topic has run its course so im gonna mark it solved.  Thank you all for sharing your insites and views. 

Dave :]