the-events-calendar / the-events-calendar-category-colors

Plugin add-on for The Events Calendar to colorize categories
https://wordpress.org/plugins/the-events-calendar-category-colors
45 stars 19 forks source link

Fix accessing undefined array indices #137

Closed IanDelMar closed 1 year ago

IanDelMar commented 2 years ago

When adding a new event category neither the text nor the background or border option are set. Therefor the indices {$slug}-text, {$slug}-background and {$slug}-border are not defined in the $options array. PHP throws undefined index notices when trying to access the array elements.

This PR proposes to check whether said indices are defined before accessing them.

afragen commented 2 years ago

I was reviewing this and I think the better solution is to ensure there are defaults set. This should happen in Main::add_defaults but I have to trace that and ensure set values aren't overwritten.

Did this error occur before you saved any settings?

IanDelMar commented 2 years ago

I noticed the notice on a live site where the notice was and is literally flooding the debug log. I tried to replicate the exact same issue in my local dev environment - I was only partly successful.

Local environment with

Live site with

Locally I was able to replicate the notice in two scenarios

On the live site the notice is thrown for each human and each bot visiting the site if a category has been added without re-saving the category color settings. Is it possible that this is somehow related to the pro version?

Concerning default values, I would prefer to apply no styles by default. For existing categories your (the plugin authors) choices for defaults are applied immediately after activating the plugin. There is no way for the plugin user to prevent styles being applied that do not match the overall look of the site. And if the user, e.g., wants to highlight one category only, they have to disable all the styles instead of just setting one style. If default values are applied immediately after creating a new category, this might also apply styles that do not match the site. That would be a breaking change but maybe you can consider this for the next major version? I agree, given that currently there are default styles, to add default styles for "unsaved settings" is the better solution.

afragen commented 2 years ago

I can probably switch the default to transparent. I regularly run on sites setting WP_DEBUG on. I don't think I've seen this particular error before.

It's clearly an issue. I'm still trying to find the best way to fix it.

IanDelMar commented 2 years ago

If it's an option to change default values, I think it would be better to have no default values. Maybe there's something I'm not aware of but that's my reasoning:

We have three properties: background color, text color, border color. Each one may already have proper styling.

I think in most cases the user is forced to change the default values or check the hide setting. Until they do default styles may clash with the site's styles. On the other hand, if there are no default styles (applied), the user will always have to change the styles (most of the users will do this anyway) but there will never be adverse effects of default styles. I'm not sure, but if you check the hide option by default or switch from not applying styles to enable styles no CSS will be generated by default, you can set whatever default value you like for the color properties because they will never be applied until the user opts in. That probably solves the undefined index problem as well.

kadamwhite commented 1 year ago

Edit: It may not be the "root cause" but the reported issue for the other ticket I opened was the log warning count, not a language-specific issue, so fixing the log warnings would fix the immediate client request.