fix(site_notifications): cast read state of site notifications to int before saving as metadata #10739
Conversation
… before saving as metadata
|
I restarted the JS travis build. |
|
This should not be fixed this way... |
|
Agree with @jdalsem. Let's cast to bool and log a notice instead of warning. |
|
No warning or notice is my preference. It is accepted by design to send booleans... you will never get them back as booleans... but that is how it has been for a long time. In a future PR we could add the feature that booleans stay booleans, but for now i do not want to see any systemerror/warning/notice |
|
i guess so... boolean being converted to int has been a long time part of how metadata worked... like i said, maybe it is a better approach to be able to convert it back to boolean, rather than converting it to ints... but that is another discussion. Bool to int conversion is by design. So it is meant to work to put booleans in metadata (do not expect to get booleans back though). Triggering all these warnings now will cause issues that were previously never there... because they were no issues. So maybe #10187 was not such a good idea... at least for booleans |
|
Silent casting led to a real bug in my code that was hard to track down; I saw some plugin storing a bool and figured I'd get that back. IMO API usability should focus on new users than the (very few) devs that know its historical quirks. Perhaps a NOTICE would be sufficient to be seen during development but likely be filtered out in production. |
|
We do have the value type column, so we could in theory make the bools work, but eh, who cares. I think a notice is the middle ground. |
|
why not silence the notice for now... (as it draws attention to non-bugs... as i always develop with ALL notices on) and make booleans work in master? Just a few more months before 3.x... |
|
I think it results in better code when we store values that we expect to get back. I agree that notices are annoying and I am guilty of just storing bools all the time, but in reality we are dealing with integers so we should store ints and not bools and encorage others to do so as well. So, I am for keeping notices. |
|
Hmmm, I'm not sure I see a consensus yet about how to proceed. I think the first thing to decide would be if storing (silently) anything but text/ints is still okay or not. If it's still okay I don't think that a warning is appropriate. Maybe a notice is okay (for example I mostly keep only errors and warning on and try to get rid of the reasons for them showing up only). Is it mentioned in the docs (if yes, prominently enough?) how metadata/annotation values are saved? Even when not reading the docs before coming across the problem in practice it might be enough to mention it in the docs - as you might check the docs when you come across the problem specifically for info about it. The problem I have with the current warning is that it won't tell you anything about the metadata/annotation that has a non-text/non-int value. If you don't know for sure that it's not your code you are currently working on causing the warning you likely waste time tracking down the origin of the warning. And if you then find out that it's Elgg core code causing the warning you will most likely be either frustrated or annoyed. I don't know if there are even other cases in Elgg core apart from this single case in site-notifications that would result in the warning to occur. If this would be the only case (or even if there would be a few others in code of Elgg core) I would suggest to fix the casting here even when not strictly necessary. Then the core code would be clean (even if saving booleans or other types is not forbidden). So, there would be no warnings on a clean install and plugin devs could still decide on their own if they just ignore the warning or cast their data. |
|
Well if we keep the warning this (or tune it down to a notice) this PR is still needed. Personally i do not want to hunt down all the (yet unknown) cases where we store booleans, but that is the consequence of keeping the notice intact. Maybe the amount of situations is just so little it is not worth discussing any further. If @hypeJunction and @mrclay feel the warning/notice should stay, this PR is ready to be merged. |
A very good point. How about I set up an issue to vote on GitHub: either I'll add the name of the metadata to the message and reduce to notice level, or we'll return to silent casting. |
|
Vote issue: #10749 |
|
No more warnings due to #10788. So, closing. |
Meant to fix #10738.