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.
Problem/Motivation
Now that there is a UI, some users should not be able to edit a YAML form's YAML source.
Proposed resolution
Provide a 'edit yamlform source' permission.
Remaining tasks
- Block users from editing YAML form source
- Block users from editing YAML form element custom settings.
- Must provide an update hook
Development Notes
- Branch Name: 2759531-edit-yamlform-source-permission
- Commit message: Issue #2759531: Provide 'edit yamlform source' permission
Comment | File | Size | Author |
---|---|---|---|
#17 | provide_edit_yamlform-2759531-16.patch | 11.84 KB | jrockowitz |
#17 | interdiff-2759531-9-16.txt | 10.17 KB | jrockowitz |
#14 | interdiff-2759531-9-14.txt | 5.47 KB | jrockowitz |
#14 | provide_edit_yamlform-2759531-14.patch | 7.07 KB | jrockowitz |
#11 | interdiff-2759531-9-11.txt | 4.49 KB | jrockowitz |
Comments
Comment #2
jrockowitz CreditAttribution: jrockowitz as a volunteer commentedComment #3
JockeL CreditAttribution: JockeL commentedMaybe this should also include: Block users from entering Default YAML form submission data? (In the form that is attached to an entity)
Comment #4
scott_euser CreditAttribution: scott_euser commentedI've created a patch to add this functionality: "Block users from editing YAML form source" and "Must provide an update hook".
Without the ui module enabled, I think it doesn't need this permission but when you enable the UI, you have the new source route. I think it makes sense to then add permission to roles which currently have permission to edit the source so I've added an update to the install file for yamlform_ui. That way if someone updates the ui module, their existing permissions are effectively maintained.
Haven't patched before so please let me know if this isn't the best method or I've done something wrong :)
Comment #5
jrockowitz CreditAttribution: jrockowitz as a volunteer commented@JockeL I think it is really important that all users can set default values for a YAML form. I think the bigger issue is that users need to be educated more about what is YAML and why Drupal 8 and this module is embracing it.
#2759591: What is YAML and why we are using it?
Comment #6
jrockowitz CreditAttribution: jrockowitz as a volunteer commented@scott_euser Yout patch looks really good. I think you can kill two birds with one stone and also hide 'custom properties'.
In \Drupal\yamlform\YamlFormElementBase::buildConfigurationForm (LINE 654) you just need to add something like
Can you also please add a test called something like \Drupal\yamlform_ui\Tests\YamlFormUiElementTest::testPermissions.
Comment #7
scott_euser CreditAttribution: scott_euser commentedSounds good, I might not get to work on the test today as I need to take off shortly, but will try to get to it as soon as possible.
Would it be wise to make two separate permissions like this patch?
Comment #8
jrockowitz CreditAttribution: jrockowitz commentedFor now I think it should be one permission. The goal is just to block certain users from editing YAML source.
Also the entity.yamform.source_form should check the permission and yamform.update access. We don't want the permission to automatically grant access to update a yamform. I am not sure if you can have both permissions in the route yaml or you will have to create a custom access callback.
Comment #9
scott_euser CreditAttribution: scott_euser commentedSounds good. I've updated the patch.
I'm not 100% sure what you mean by your second paragraph. Access is only granted in the update if they already have the 'administer yamlform' access (since before the patch, they'd have access to the source, this then maintains that access).
Comment #10
scott_euser CreditAttribution: scott_euser commentedAh and I also added a testPermissions as you suggested. Still learning D8 so I'm not sure if I need to add it somewhere else so the test gets automatically run or if adding the method there is enough.
Comment #11
jrockowitz CreditAttribution: jrockowitz commentedNo problem, I am new at reviewing code. Your tests are running fine.
I have also included an interdiff so that you can see my changes.
Comment #12
jrockowitz CreditAttribution: jrockowitz commentedBTW Dreditor makes reviewing the interdiff much easier
Comment #14
jrockowitz CreditAttribution: jrockowitz commented..and I wrote a broken patch.
Comment #15
jrockowitz CreditAttribution: jrockowitz commentedComment #17
jrockowitz CreditAttribution: jrockowitz commentedI weep sadly as my patch fails again...
Comment #18
scott_euser CreditAttribution: scott_euser commentedAh now I see what you meant by the custom access. Yeah makes sense to check against whether they also have permission to update.
Also makes sense to have the hook install in both yamlform and yamlform_ui.
Thanks for the Dreditor tip, very useful!
Comment #21
jrockowitz CreditAttribution: jrockowitz commented@scott_euser Done! Thanks for your help with this issue.
Comment #22
scott_euser CreditAttribution: scott_euser commentedExcellent, thank you! Happy to have this in place!
In a few days I should be able to try to take a stab at the UI for managing options
Comment #23
jrockowitz CreditAttribution: jrockowitz commented@scott_euser. Let's talk before you start working on the options UI. There is a very specific approach I want to try doing so that the code is reusable for the states ui
Comment #24
scott_euser CreditAttribution: scott_euser commentedSounds good, let's move any notes on it to here: https://www.drupal.org/node/2759527