search

opencaching / opencaching-pl

The source code of Opencaching.PL (and some other domains)
https://opencaching.pl/
GNU General Public License v3.0
22 stars 33 forks source link

Prevents passing empty lat or lon in a new cache creation #2323

Closed rapotek closed 2 years ago

rapotek commented 2 years ago

From OCPL forum:

zdaje się, że chwilowo istnieje jakiś błąd, który pozwala na dodanie skrzynki bez kordów a to z kolei wywala stronę główną.

It looks like passing out empty coordinates in a new cache creation, allowed until now, caused a crash on the main page. This fix removes a possibility to store empty (null) values as a new cache coordinates.

It is a simple fix (just if...else removal), without any php style checking.

But one question arises: why entering zeros (i.e 00.000) in latitude or longitude for a new cache is forbidden while it is allowed in a cache edition? I did not change this functionality but I would like to know its purpose.

andrixnet commented 2 years ago

Don't all caches by definition have coordinates?

All zeroes, though geographically meaningless, represents valid data. However IMO should not be default coord fields contents. Fields should be empty for the user to fill out and with proper input checking.

On 2021-11-15 1:48 PM, rapotek wrote:

From OCPL forum:

zdaje się, że chwilowo istnieje jakiś błąd, który pozwala na
dodanie skrzynki bez kordów
a to z kolei wywala stronę główną.

It looks like passing out empty coordinates in a new cache creation, allowed until now, caused a crash on the main page. This fix removes a possibility to store empty (null) values as a new cache coordinates.

It is a simple fix (just |if...else| removal), without any php style checking.

But one question arises: why entering zeros (i.e 00.000) in latitude or longitude for a new cache is forbidden while it is allowed in a cache edition? I did not change this functionality but I would like to know its purpose.

    You can view, comment on, or merge this pull request online at:

  https://github.com/opencaching/opencaching-pl/pull/2323

    Commit Summary

(1 file https://github.com/opencaching/opencaching-pl/pull/2323/files)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opencaching/opencaching-pl/pull/2323, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZZ36NY3RI7SNDSWBVJSK3UMDXQLANCNFSM5IBPQNXA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rapotek commented 2 years ago

Don't all caches by definition have coordinates? All zeroes, though geographically meaningless, represents valid data. However IMO should not be default coord fields contents. Fields should be empty for the user to fill out and with proper input checking.

I agree, that there should not be defaults for coordinates, and it has been in this way for a while. But it is also possible in working code to omit entering coordinates at all, what results in NULL values stored in database for coordinates. This PR keeps empty initial values for coordinate fields but enforces filling them up while creating a new cache.

Forbidden zero values for coordinates is another thing. I do not know its purpose so I left it as is was, but I suspect it is a kind of old protection against invalid data. However, why this "protection" exists in a new cache creation only and in edition you can freely enter zeroes?

kojoty commented 2 years ago

Yes, all-zeroes is in fact very less possible as this is located at ocean near Africa but this is a problem with proper handling of zeros in the code - of course if everything is written ok it should not be a problem ;)

rapotek commented 2 years ago

Yes, all-zeroes is in fact very less possible as this is located at ocean near Africa but this is a problem with proper handling of zeros in the code - of course if everything is written ok it should not be a problem ;)

The code does not allow to create a cache with either 0 lat or 0 lon, they are checked independently. You cannot create a cache on the Greenwich meridian as well as on the equator. I wonder why nobody from OCUK ever complained ;). BTW, posted data are anyway checked in details about numeric, range etc. Maybe there is a specific case which I don't see and it requires this "zero" comparison, but if not, it looks like being too strict.

andrixnet commented 2 years ago

Well, longitude 0 0.000 by itself (with various lat values) is a perfectly ok value. The simple check that both lat and lon are zeroes is another story.

On 2021-11-17 4:16 PM, rapotek wrote:

Yes, all-zeroes is in fact very less possible as this is located
at ocean near Africa but this is a problem with proper handling of
zeros in the code - of course if everything is written ok it
should not be a problem ;)

The code does not allow to create a cache with either 0 lat or 0 lon, they are checked independently. You cannot create a cache on the Greenwich meridian as well as on the equator. I wonder why nobody from OCUK ever complained ;). BTW, posted data are anyway checked in details about numeric, range etc. Maybe there is a specific case which I don't see and it requires this "zero" comparison, but if not, it looks like being too strict.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/opencaching/opencaching-pl/pull/2323#issuecomment-971623351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZZ36LTEQPV3AHHJBVT223UMO2NLANCNFSM5IBPQNXA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rapotek commented 2 years ago

@deg-pl ignored this PR and made a client-side validation, so it won't bother me anymore. The Prime meridian problem (OCUK f.ex.) in newcache still remains, and not only there.