locgn38 / GeolocOptinAdhoc

Géolocalisation consentie (opt-in) contextualisée (ad-hoc) et temporaire (éphémère)
Other
22 stars 10 forks source link

Feedback #1

Open root-io opened 8 years ago

root-io commented 8 years ago

Hey great work!

Do you consider switching to the PHP framework Symfony (https://symfony.com/) ? It will improve the development (a lot).

And maybe separate the client from the API and build it with something like ReactJS or Angular2 ?

Plus, I don’t know if you’re aware but chrome started to block non-ssl HTML5 geolocation (won’t work without https) Source: https://developers.google.com/web/updates/2016/04/geolocation-on-secure-contexts-only


Hey joli travail!

Est-ce que vous seriez pour, utiliser un framework PHP comme Symfony (https://symfony.com/) ? Cela améliorerait (beaucoup) le developpement.

Et peut-être séparer le client de l'API et le dev avec une techno du style ReactJS ou Angular2 ?

Aussi, Je ne sais pas si vous êtes au courant mais chrome a commençé à bloquer la géolocalisation HTML5 pour les connexions non SSL (fonctionne seulement avec https) Source: https://developers.google.com/web/updates/2016/04/geolocation-on-secure-contexts-only

Spomky commented 8 years ago

Hi,

I read about that project on Numerama (in French) and that project looks very interesting.

As @root-io, I think it could be improved by using modern framework (Symfony is a good choice), tools (e.g. Composer) and Coding Practices (Semantic Versioning, PHP-Fig Recommandations implementation...). IMHO the API is only needed if the project becomes a platform for many Safety Services and will suppose it is managed at a higher level (ministry/department). Nevertheless we can keep that in mind for futur releases.

I forked it and ran some security checks and code reviews and the result is that major and critical issues exist and have to be taken into account (see results here). I also noted that it uses postgresql+postgis. Such project should work with any other ORM and, I may be wrong, but I do not see the point of using postgis: the projet is only supposed to get the user location, store it in the DB and show it in the back office. There is no specific calculations to perform.

My proposal to update that project is to perform the following actions:

What do you think?


Hi,

J'ai entendu parler de ce projet sur Numerama et ça a l'air vraiment intéressant.

Comme @root-io, je pense qu'il peut être amélioré en utilisant un framework moderne (Symfony est un bon choix), des outils modernes (ex. : Composer) et de bonne pratique de codage (Semantic Versioning, mise en place des recommendations du PHP-Fig...). À mon avis l'API est seulement nécessaire si le projet devient une plateforme pour différents services (Pompiers, Gendarmerie ou autres) et suppose qu'il est géré à un niveau plus élevé (ministère/organisme d'État). Néanmoins c'est une idée à garder pour des évolutions futures.

J'ai dupliaué le projet et lancé quelques analyses du code. Le résultat et que des bugs importants voire critiques existent et doivent être pris en compte (voyez les résultats ici). J'ai aussi noté qu'il utilise postgresql+postgis. Ce type de projet devrait fonctionner avec d'autres gestionnaires de base de données. Je me trompe peut-être, mais je ne vois pas l'uilité de postgis : le projet ne fait que récupérer, enregistrer et afficher la localisation des usagers. Il n'y a aucun calcul spécifique à réaliser.

Mon idée pour mettre à jour ce projet et de procéder comme suit

Qu'en pensez-vous?

root-io commented 8 years ago

I read it on Numerama too.

Interesting, I didn't know about SensioLabs Insight. I checked the results, for me their is nothing worth to fix, everything (not the htaccess one) seem related to third-party libraries (thecallR and restpostgis).

I either don't get it why the use of PostGIS, I guess we only use points, no need for calculations, store polygons or other geometry types, a float for the lat/lng in database is more than enough no ? I have no knowledge about how PostgreSQL works, can't help on that point.

@Spomky I agree with your proposal but I won't do anything until a member of this projet answer. I'm a bit rusty with PHP/Symfony, but it should be okay 😇

Spomky commented 8 years ago

I checked the results, for me their is nothing worth to fix, everything (not the htaccess one) seem related to third-party libraries (thecallR and restpostgis).

You are right it should quite easy to fix these issues, even if they come from third-party libraries (by the way those 3rd paty libraries should be updated).

a float in database is more than enough no

Yes signed floats (for N/S & E/W distinction) are enough: N 40° 45´ 36" => N 40° 45.6’ => N 40.76° we just have to store 40.76.

I have no knowledge about how PostgreSQL works, can't help on that point.

Me too, but Doctrine does so if the model is correctly set we should not care of the ORM.

I won't do anything until a member of this projet answer.

Agreed.


I checked the results, for me their is nothing worth to fix, everything (not the htaccess one) seem related to third-party libraries (thecallR and restpostgis).

Vous avez raison il doit être assez simple de régler ces bugs, même s'ils proviennent de bibliothèques tierces (qui doivent être mises à jour par la même occasion).

a float in database is more than enough no

Oui des flottants signé sont suffisants (signe pour distinguer N/S & E/O): N 40° 45´ 36" => N 40° 45.6’ => N 40.76° nous enregistrons seulement 40.76.

I have no knowledge about how PostgreSQL works, can't help on that point.

Moi non plus, mais Doctrine sait et si le model est défini correctement nous ne devrions pas nous en soucier.

I won't do anything until a member of this projet answer.

D'accord.