Problem/Motivation

In #3321861: Change image_captcha submodule and test modules location., a title config setting was provided. But a corresponding config schema wasn't added. Let's add it.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 3356063.patch981 bytesheddn

Issue fork captcha-3356063

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Active » Needs review
StatusFileSize
new 3356063.patch981 bytes
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil
Issue summary: View changes

Thanks @heddn! @Grevil could you have a look, please? If someone could provide a MR earlier, that would be super nice :)

Edit: X-Post! Thanks for the patch! :)

Grevil made their first commit to this issue’s fork.

grevil’s picture

grevil commented
Status: Needs review » Needs work

Thank you, @heddn! I think the update hook is not properly implemented, sorry!

There shouldn't be an "image_captcha" "title" config key. Instead, the "captcha" "title" value should've been set there.

Grevil opened merge request !62

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs work » Needs review

Fixed, please review.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Title: image_captcha.settings.title schema missing » image_captcha_update_9002 incorrectly updated image_captcha instead of captcha title

Should this update be moved from image_captcha into captcha?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil

@Grevil could you have a look again, please?

grevil’s picture

grevil commented

Since we are removing config from image_captcha and adding it to captcha, we need to have both modules activated. We could also only remove the config from image_captcha and be done with it, without touching "captcha", so if people renamed their captcha title to something else, it won't be reset to "CAPTCHA". Either way, this is image_captcha specific.

EDIT: To be safe, we should not change the current patch.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3354456: Thumbnails of the fonts are vanished in the settings page after updating to 2.0.0-beta1
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: grevil » Unassigned
Status: Needs review » Reviewed & tested by the community

Thank you @heddn and @Grevil! Changes LGTM, #10 sounds reasonable to me. As it was wrongly added in image_captcha, I think it should also be removed in there.

Let's get this fixed! :)

  • Anybody committed 1bd84f88 on 2.x authored by Grevil 
    Issue #3356063 by Grevil, heddn: image_captcha_update_9002 incorrectly...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

Merged, thank you!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: -#3354456: Thumbnails of the fonts are vanished in the settings page after updating to 2.0.0-beta1
grevil’s picture

grevil commented
Status: Fixed » Needs work

As stated in #10 the patch is generally fine as is, BUT we should create an update hook, warning the user, that their potential custom Captcha title will be replaced with "Captcha" and that they have to rename it accordingly after the update.

grevil’s picture

grevil commented
Status: Needs work » Fixed

Creating a follow-up issue.

grevil’s picture

grevil commented

See #3365278: [Follow-up] image_captcha_update_9002 incorrectly updated image_captcha instead of captcha title.

  • Anybody committed 32c129e4 on 2.x 
    Revert "Issue #3356063 by Grevil, heddn: image_captcha_update_9002...
grevil’s picture

grevil commented

We will adjust the test properly inside #3365278: [Follow-up] image_captcha_update_9002 incorrectly updated image_captcha instead of captcha title.

grevil’s picture

grevil commented
Related issues: +#3365278: [Follow-up] image_captcha_update_9002 incorrectly updated image_captcha instead of captcha title
grevil’s picture

grevil commented

@heddn you are completely correct, the update hook should have never existed in image_captcha, but rather in captcha...

Sry, it seems like I over read your comment in #8.

We will fix everything in the follow up issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.