Smiley settings problems on upgrade

[sbulen]

The logic for adding the 2.1 Alienine & Fugue smiley sets to smf_settings is failing. I believe Fugue is failing because it's the default, and there is already a default. Alienine is failing because of syntax. Note the directories are all there in the filesystem, but the names are confused because the smf_settings values are confused:

The logic for updating these values in the upgrader needs to be fixed.

Another problem: In 2.0, the default was alienine, so in the /default folder are alienine .gifs. In 2.1, the default is fugue, so in the /default folder are fugue .pngs. As a result, when installing the 2.1 files on top of the 2.0 files, you now get a mix of alienine & fugue in the /default folder. I suggest leaving the default at alienine, so the /default folder would include alienine .gifs & .pngs post installation.

Another consideration... Some folks may want to just delete the Smileys folder prior to upgrading. If this were to happen, the old 2.0 values in the settings table would point to missing folders & smileys. It might be helpful to confirm they exist. (We did this with themes, over here: https://github.com/SimpleMachines/SMF2.1/pull/3964/files)

This issue was split from #4767 .

[Sesquipedalian]

We should also stop storing the file extension in the database table at all. Instead of updating the table value from, e.g., smiley.gif to smiley.png, we should update to just smiley and then determine the correct file extension at runtime.

[albertlast]

• In my eyes is the file "extension" the filename
• To "determine" mean we do a bruteforce -> slow down the the runtime

In short i don't like your idea.

[Sesquipedalian]

Well, the only options are:

1. To force all smiley images to use one file format (PNG).
2. To check for existing files with different possible extensions at runtime.
3. To radically revise the entire parsesmileys() logic and alter the structure and contents of the smileys database table.

[albertlast]

the secondone doesn't work in the long run (which smiley we choose when exists with many different extension) -> is more or less random
1 or 3 are possibles ways,

[Sesquipedalian]

Well, the way I wrote the code in #4772 wouldn't be random. It will always prefer an SVG, then a PNG, then a GIF, and lastly a JPEG. So if there is both a PNG and GIF in a given smileys directory, the PNG will always be chosen.

But still, you are right that doing file system checks at runtime is not ideal. Since option 1 is undesirable (it is the equivalent of leaving #4767 unsolved), the only really good, long-term solution is option 3.

[Sesquipedalian]

Okay, I am now convinced that option 3 is worth doing. I'll put together a PR sometime in the coming couple of weeks to address it and the other issues @sbulen has identified in this bug report.

[albertlast]

but when other image format gets better/more used (like avif) you had to change the code and set the right "order",
reason why i don't like the second way and all so you pr not.

When the smiley is not found i would use the blank one: https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Smileys/default/blank.png as fallback.

[Sesquipedalian]

but when other image format gets better/more used (like avif) you had to change the code and set the right "order",
reason why i don't like the second way and all so you pr not.

As I said, you are right that doing file system checks at runtime is not ideal.

When the smiley is not found i would use the blank one: https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Smileys/default/blank.png as fallback.

That would still leave GIF smileys unsupported. If we want GIF smileys to actually work, we need to go with option 2 or 3. #4772 implements a version of option 2, and can serve as a temporary solution until I make a new PR to implement option 3.

[sbulen]

@Sesquipedalian - if you like I can tackle the installer & upgrader changes. I can have a PR out there within the next few days.

My initial thought is to store the extensions as simple json pairs within smiley_sets_known - so each directory has a corresponding extension. I can set those values - legacy are .gif, the two new ones are .png.

That leaves the ACP & parse_smileys to you. If you agree, I'll get started.

A wrinkle: Not 100% confirmed yet, but I believe part of the problem is that the 2.0 upgrader file is mangling the smiley set data. The upgrader runs both the 2.0 file & the 2.1 file. It has always done so, by design, to ensure that the DB is truly 2.0 to start with. I don't think the 2.0 smiley step was designed to be rerun.

[sbulen]

The wrinkle I mentioned above is confirmed. The "don't do this twice" logic in upgrade_2-0_mysql.sql isn't working:

Settings entries: 2.0 - after vanilla install
smiley_sets_default:	default
smiley_sets_known:	default,aaron,akyhne
smiley_sets_names:	Alienine's Set Aaron's Set Akyhne's Set

Settings entries: Mid 2.1 upgrade - vanilla (2.0 smiley sets added 2nd time by upgrade_2-0_mysql.sql)
smiley_sets_default:	default
smiley_sets_known:	default,aaron,akyhne,aaron,akyhne
smiley_sets_names:	Alienine's Set Aaron's Set Akyhne's Set Aaron Akyhne

Settings entries:  Post 2.1 upgrade - vanilla
smiley_sets_default:	default
smiley_sets_known:	default,alienine
smiley_sets_names:	Fugue's Set Alienine's Set

Found here: https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/other/upgrade_2-0_mysql.sql#L2823

[Gwenwyfar]

Do we really need to have a folder named "default"? Seems to make more sense to me to leave all folders with smiley set names and only change default in the settings. Same for the text strings. Also, I'm against leaving alienine as the default. Half the purpose of changing it was so we'd have a nicer default set. Unless it's way too much of a bother to fix...

[sbulen]

We can set it up however we want!

We just need to anticipate the fact that folks installing 2.1 on top of 2.0 will have that /default folder, with alienine .gifs in it. The current code dumps fugue .pngs alongside those in the /default folder.

I like your idea of each set having a distinct name & getting rid of the /default folder.

I'll get started on the installer & upgrader changes.