Make WordPress Core

Opened 21 months ago

Closed 17 months ago

Last modified 17 months ago

#55900 closed defect (bug) (fixed)

Installer screen, when submitting a faulty form the search engine checkbox is reset

Reported by: ramon-fincken's profile ramon fincken Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-screenshots has-patch
Focuses: ui Cc:

Description

When submitting with an empty field, the search engine checkbox is reset to zero

Attachments (5)

screencapture-localhost-wordpress-make-src-wp-admin-install-php-2022-06-02-13_10_52.png (129.1 KB) - added by ramon fincken 21 months ago.
Checked before submit
screencapture-localhost-wordpress-make-src-wp-admin-install-php-2022-06-02-13_11_54.png (106.2 KB) - added by ramon fincken 21 months ago.
Unchecked after submit
55900.1.patch (776 bytes) - added by ramon fincken 21 months ago.
patch 55900.1
55900.2.patch (1011 bytes) - added by rafiahmedd 21 months ago.
I just found a better way to do this.
55900.diff (599 bytes) - added by khokansardar 21 months ago.
Here is the proper PR of the task

Download all attachments as: .zip

Change History (32)

#1 @audrasjb
21 months ago

  • Focuses ui added
  • Milestone changed from Awaiting Review to 6.1
  • Owner set to whaze
  • Status changed from new to assigned
  • Version trunk deleted

Hello @ramon-fincken and thanks for the ticket!

Let's fix this in the next major :)

Assigning the ticket to @whaze who volunteered to handle this during WCEU contributor day!

#2 @ramon fincken
21 months ago

Prepping patch file now :)

@ramon fincken
21 months ago

patch 55900.1

#3 @ramon fincken
21 months ago

  • Keywords has-patch added; needs-patch removed

#5 @whaze
21 months ago

Hello from contributor's day,
i added a PR

This ticket was mentioned in PR #2772 on WordPress/wordpress-develop by itzmekhokan.


21 months ago
#6

Trac ticket: 55900

@rafiahmedd
21 months ago

I just found a better way to do this.

@khokansardar
21 months ago

Here is the proper PR of the task

#7 @ramon fincken
21 months ago

I verified that the checked function does NOT work for this one. I agree that the shorter PHP is better. I will make a new patch.

3 cases:
1) first open of the page (should NOT be checked)
2) submit when checked (should be checked)
3) submit when NOT checked (should be NOT checked)

#8 @ramon fincken
21 months ago

update: I cannot get to work the 2nd patch, so sticking with the first one

the checked function is really funky as we need to test something that has value === 0

#10 @khokansardar
21 months ago

Hi @ramon-fincken please check with my patch https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/2772.diff that I have already added. Its working and I guess its a proper PR for this ticket.

#11 @audrasjb
21 months ago

PR2777 looks good to me: https://github.com/WordPress/wordpress-develop/pull/2777
Are we good to ship this version of the patch?

#13 @audrasjb
21 months ago

  • Keywords needs-testing added

Hey there,
I refreshed PR2777 so it passes unit tests.
https://github.com/WordPress/wordpress-develop/pull/2777

This PR includes the conditional statement proposed by @ramon-fincken.
Looks like it works in the 3 cases listed by Ramon.

#14 @khokansardar
21 months ago

  • Keywords changes-requested dev-feedback 2nd-opinion added

Hey I think instead of checking again $_POST['blog_public'] and storing it in $checked variable and print it, we should modify the variable $blog_public when it's defined and assign the value based on $_POST['blog_public'] data.
So as per logic it should be something like my above PR - https://github.com/WordPress/wordpress-develop/pull/2772.
This PR also works in the 3 cases listed by Ramon.

#15 @costdev
19 months ago

  • Keywords 2nd-opinion removed

I concur with @khokansardar's suggestion that PR 2772 is a better approach.

It's also worth noting that when the blog_privacy_selector action exists, PR 2777 will currently fail to maintain the previously selected option.

This would require

<?php
// PR 2777: Line 198
$checked = ( isset( $_POST['blog_public'] ) && $blog_public ) ? ' checked="checked"' : '';

to be moved to ~Line 186 and $checked also used on Lines 189 and 191. Rather than making multiple changes here, PR 2772 makes a single change that works with and without the blog_privacy_selector action being used.

To test the blog_privacy_selector, you can drop this into a Must-Use plugin prior to visiting the install screen.

<?php

/**
 * Plugin Name: Blog Privacy Selector
 * Description: Enables radio options for search engine visibility.
 * Author:      WordPress Core Contributors
 * Author URI:  https://make.wordpress.org/core
 * License:     GPLv2 or later
 * Version:     1.0.0
 */

// Comment this out to test without the blog privacy selector.
add_action( 'blog_privacy_selector', '__return_empty_string' );
Last edited 19 months ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


18 months ago

#17 @costdev
18 months ago

@audrasjb If you have time, can you take a look at my comment above and let me know your thoughts? Thanks!

#18 @Ankit K Gupta
17 months ago

Hello All, I tested this issue from a user perspective and used PR https://github.com/WordPress/wordpress-develop/pull/2772 . This PR fixed the issue and Installation screen is working as expected.

Test Report

Env

Test result

Working expected with PR https://github.com/WordPress/wordpress-develop/pull/2772. Search engine checkbox appears as selected (if user checked it) after the validation occurs.

Screencast example:

Before Fix -

https://i.imgur.com/yS8CqhY.jpg

After Fix -

https://i.imgur.com/d8sBtXt.jpg

#19 @audrasjb
17 months ago

  • Keywords needs-testing changes-requested dev-feedback removed

Ok the logic in PR2772 looks good to me.

Marking as ready for commit (also, self assigning).

#20 @audrasjb
17 months ago

  • Keywords commit added
  • Owner changed from whaze to audrasjb
  • Status changed from assigned to accepted

#21 @audrasjb
17 months ago

  • Keywords commit removed

Hmm. Looking closely, we should probably escape/cast the POST value here?

$blog_public = isset( $_POST['blog_public'] ) ? $_POST['blog_public'] : $blog_public;

#22 @khokansardar
17 months ago

Hi @audrasjb I have updated my PR and added value cast for POST data, here you can check - https://github.com/WordPress/wordpress-develop/pull/2772/

#23 @audrasjb
17 months ago

Looks good to me, thanks!

#25 @audrasjb
17 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 54326:

Upgrade/Install: Keep search engine checkbox value when reloading the Install screen.

This changeset ensures the value of the search engine checkbox is not reset to its default value when a faulty form is sent in the Install screen.

Props ramon-fincken, audrasjb, whaze, rafiahmedd, khokansardar, costdev, ankit-k-gupta.
Fixes #55900.

#26 @audrasjb
17 months ago

Yes @ramon-fincken as far as I know, I tested it and it passes the 3 test cases.

Note: See TracTickets for help on using tickets.