Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We should move the title field to the first form page, or move the machine name to the second.
Comment | File | Size | Author |
---|---|---|---|
#24 | flag_machine_name_field-1689400-23.patch | 7.33 KB | Scyther |
#21 | flag_machine_name_field-1689400-21.patch | 5.92 KB | Scyther |
#12 | flag_machine_name_field-1689400-12.patch | 5.52 KB | Scyther |
#9 | flag_machine_name_field-1689400-9.patch | 6.37 KB | Scyther |
#7 | flag_machine_name_field-1689400-7.patch | 6.75 KB | Scyther |
Comments
Comment #1
joachim CreditAttribution: joachim commentedComment #2
Scyther CreditAttribution: Scyther commentedPatch:
- Moves the title field to the first add flag form and changes the name field to machine-name type.
- When going from the first form page to the second one, when adding a new flag, the flag is stored temp in a cache table with ctools. (Taken insperation from Views)
- Adds dependencies for ctools for using the cache meantion above.
- Not allowed to change the name (machine-name) after the flag is saved. Good or bad?!?!
- The url for the second form when adding flag has been changed from
admin/structure/flags/add/*type*/*name*
toadmin/structure/flags/add/*name*
because the type is not longer needed because it is saved in the temporary cached flag.There is things that could be improved and I will try to get time to work on thoose and make a new patch. Please give some feedback.
Comment #4
Scyther CreditAttribution: Scyther commentedNew patch with updated DrupalWebTestCase.
Comment #6
joachim CreditAttribution: joachim commentedThanks for working on this!!
> the flag is stored temp in a cache table with ctools. (Taken insperation from Views)
> - Adds dependencies for ctools for using the cache meantion above.
Ouch. Do we really need to add a dependency just for this?
I know CTools is pretty popular now since Views needs it (and who builds a site without views) but adding the dependency just for a UI tweak seems a bit much.
I'm wondering whether the approach ought to be to take the name field off the type selection form entirely, and have a menu callback for the flag add form that takes a type argument. Would need some path tweaking:
- '/manage/%flag/edit' edits a named, existing flag
- 'add/TYPE' would need to present the edit form but for a new, empty flag.
Comment #7
Scyther CreditAttribution: Scyther commentedHere is a new patch without the dependency for CTools.
Sounds good, will take a look a this and make a new patch.
Comment #9
Scyther CreditAttribution: Scyther commentedPatch with thoose changes you proposed.
Patches that I have made so far, you can't change the name (machine name) of the flag after it's saved. Good or bad?
Comment #10
socketwench CreditAttribution: socketwench commentedPersonally, I find "Flag name" confusing for the #title. I'd rather it read "Machine name". What does View's do?
EDIT: I'm also concerned that this patch would blow up when #1681540: replace 'content' with 'entity', eg $content_type with $entity_type gets in.
Comment #11
joachim CreditAttribution: joachim commentedYikes! Bigger patch than I thought!
> Personally, I find "Flag name" confusing for the #title. I'd rather it read "Machine name".
I agree.
This needs to move to the flag edit form.
IIRC we currently allow changing of the machine name, with caveats. So I'd say we should keep that functionality.
> EDIT: I'm also concerned that this patch would blow up when #1681540: replace 'content' with 'entity', eg $content_type with $entity_type gets in.
Everything's going to blow up then.... :)
Comment #12
Scyther CreditAttribution: Scyther commented> Personally, I find "Flag name" confusing for the #title. I'd rather it read "Machine name".
Fixed
> IIRC we currently allow changing of the machine name, with caveats. So I'd say we should keep that functionality.
Fixed
> This needs to move to the flag edit form.
Does it? Machine name field has it own validation and does it same as $flag->validate_name() does. See form_validate_machine_name()
Also in in flag_form_validate(), $flag->validate() is called which calls $this->validate_name().
Comment #13
joachim CreditAttribution: joachim commented> Does it? Machine name field has it own validation and does it same as $flag->validate_name
Oh. Yes, you're right. Sorry!
Comment #14
socketwench CreditAttribution: socketwench commentedLooks good to me.
Comment #15
socketwench CreditAttribution: socketwench commentedAnd committed. http://drupalcode.org/project/flag.git/commit/2127cdd
Comment #16
joachim CreditAttribution: joachim commentedOn 3.x? I really hope that doesn't break the monster rename patch...!
Comment #17
joachim CreditAttribution: joachim commentedThis breaks my local branch at #1681540: replace 'content' with 'entity', eg $content_type with $entity_type.
I'm afraid I think we're going to have to revert this, then get 1681540 in, then come back to this. Sorry!
Comment #18
joachim CreditAttribution: joachim commentedReverted the commit. Sorry!
Comment #19
Scyther CreditAttribution: Scyther commentedNo problem. I will update my patch as soon as the "monster patch" is applied :)
Comment #20
socketwench CreditAttribution: socketwench commentedMy fault, should have realized that would happen. >_<
Comment #21
Scyther CreditAttribution: Scyther commentedUpdate patch to work with the new commits from #1681540: replace 'content' with 'entity', eg $content_type with $entity_type
Comment #22
joachim CreditAttribution: joachim commentedLooking good!
Could you change the title on the first add page to 'Select flag type' please? And then I think we're good to go!
(Another tweak we could make is to move the radio buttons description up to hook_help() text, but that's something to tackle in another issue: #1718744: move description on flag type radio buttons to hook_help.)
Comment #23
Scyther CreditAttribution: Scyther commentedPatch updated.
- 'Select flag type' title on the first add page.
- 'Add new @type flag' title on the second add page.
- Changed title for the action link for adding new flag, from 'Add a new flag' to 'Add flag'.
- Changed value for the submit button on the first add page from 'Add flag' to 'Continue'.
Comment #24
Scyther CreditAttribution: Scyther commentedAnd here is the patch
Comment #25
joachim CreditAttribution: joachim commentedThanks!
I'm committing this with a fix for the couple of tabbed lines and whitespace changes that crept in.
For future reference, do resist the urge to fix other stuff in a patch -- such as this which as far as I can tell just makes the variable name nicer:
and fixing the strtolower to drupal_strtolower.
Those are all good fixes, but remember the kittens! :)
- Issue #1689400 by Scyther: Changed flag form to use machine name form element.
- by Scyther: Fixed use of strtolower().