Make WordPress Core

Opened 10 years ago

Last modified 2 months ago

#33600 new enhancement

Add `theme_mods_{$stylesheet}` option during `populate_options()`

Reported by: dlh's profile dlh Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch has-unit-tests needs-test-info
Focuses: Cc:

Description

After switch_theme(), a theme_mods_{$stylesheet} option is created if one doesn't exist to avoid extra database queries (if I understand #14828 correctly).

No similar option is currently created during installation, so a fresh install can includes those queries if the default theme looks for the mods before the option is added.

The attached patch would add the option during populate_options().

I included a unit test, although I'm not sure where the best location for it would be.

Attachments (1)

33600.patch (1.1 KB) - added by dlh 10 years ago.

Download all attachments as: .zip

Change History (11)

@dlh
10 years ago

#1 @swissspidy
8 years ago

  • Keywords has-patch has-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to Future Release

This ticket was mentioned in Slack in #themereview by joyously. View the logs.


7 years ago

#3 @joyously
7 years ago

Hmm, my theme actually uses the fact that it is not populated to supply default values and know that it is brand new.

This ticket was mentioned in PR #8525 on WordPress/wordpress-develop by @SirLouen.


4 months ago
#4

Adapted a 10-year-old ticket by @dlh

Trac ticket: https://core.trac.wordpress.org/ticket/33600

#5 @SirLouen
4 months ago

  • Keywords needs-testing-info added; needs-testing removed

@dlh patch was not applying so I adapted it.
The unit test is passing correctly after introducing the mods, but still, I cannot figure out the purpose of all this.

Could you provide some testing instructions to reproduce the potential issue with and without unit-tests?

#6 follow-up: @dlh
4 months ago

The problem occurs when get_theme_mod() is called before set_theme_mod() is, if it ever is. The behavior was described in more detail in #14828, and it was fixed there for theme activations, but not new installations.

The wp_get_custom_css_post() function, introduced in 4.7, saves a theme mod, and since that function is called regularly, the change proposed in this ticket has little practical impact today, although I still think it's technically sound.

#7 in reply to: ↑ 6 @SirLouen
4 months ago

  • Keywords close added; has-patch has-unit-tests needs-testing-info removed

Replying to dlh:

The problem occurs when get_theme_mod() is called before set_theme_mod() is, if it ever is.

This is one topic I'm commenting lately in the core test group. Many patches are coming too theoretical without a real use case. And are sometimes so entangled, that as you say "if it ever is", can be, technically never or even, as I've seen in some patches, in a wrong coding scenario (not sure which is this case).

the change proposed in this ticket has little practical impact today, although I still think it's technically sound.

So then we can say we can confirm to close this very old ticket, because it's not necessary anymore. If someone happens to locate an issue in the future, can just re-open the ticket.

#8 follow-up: @dlh
4 months ago

  • Keywords has-patch has-unit-tests added

I have sympathy for your point, but I think #14828 demonstrated that the behavior in question is a real bug, not just a theory or the result of developer error.

Anyway, I'll leave the close keyword and let a committer or someone else decide what to do, but I restored the other keywords, because the ticket still has a patch and tests :)

#9 in reply to: ↑ 8 @SirLouen
4 months ago

  • Keywords needs-testing-info added; close removed

Replying to dlh:

I have sympathy for your point, but I think #14828 demonstrated that the behavior in question is a real bug, not just a theory or the result of developer error.

Anyway, I'll leave the close keyword and let a committer or someone else decide what to do, but I restored the other keywords, because the ticket still has a patch and tests :)

@dlh makes sense, if this can happen we should get it fixed. I can't visualize any scenario where this can trigger the bug

Could provide a use-case with a micro plugin or simply some code snippet, where we can test the error being triggered?

PS: Btw I don't understand why but the report is marked as enhancement, not as bug

Last edited 4 months ago by SirLouen (previous) (diff)

#10 @SirLouen
2 months ago

  • Keywords needs-test-info added; needs-testing-info removed

Updating keyword to new version.

Note: See TracTickets for help on using tickets.