Commit message
Issue #1998582 by damiankloip, brantwynn, dawehner, xjm, effulgentsia: Auto-generate machine_name for Views Blocks using [viewname]_[displayname] rather than forcing manual entry.
Problem/Motivation
Currently, users must type in a custom machine_name when placing a new Views block instance.
Normally, a block machine_name is generated from the label. This doesn't work for Views blocks since ViewsBlock:form() unsets this property. See: #1957276: Let users set the block instance title for Views blocks in the Block UI
Drupal should handle assigning the machine_name for the user, generating it based on the View name and the display that the block is using (ie: block_1).
Views sets up its own plugin portions of the block configuration form but assigning machine names is not handled by the plugin. This happens afterward, when the block system hardcodes this in BlockFormController:form()
...
/**
* Overrides \Drupal\Core\Entity\EntityFormController::form().
*/
public function form(array $form, array &$form_state) {
$entity = $this->entity;
$form['#tree'] = TRUE;
$form['id'] = array(
'#type' => 'value',
'#value' => $entity->id(),
);
$form['settings'] = $entity->getPlugin()->form(array(), $form_state);
$form['machine_name'] = array(
'#type' => 'machine_name',
'#title' => t('Machine name'),
'#maxlength' => 64,
'#description' => t('A unique name to save this block configuration. Must be alpha-numeric and be underscore separated.'),
'#default_value' => $entity->id(),
'#machine_name' => array(
'exists' => 'block_load',
'replace_pattern' => '[^a-z0-9_.]+',
'source' => array('settings', 'label'),
),
'#required' => TRUE,
'#disabled' => !$entity->isNew(),
);
...
Proposed resolution
Talking to xjm, effulgentsia and dawehner, we've determined a patch should provide a form_alter in views.module that gives Views this special ability to assign auto-generated machine name for its blocks.
Related Issues
- #1938062: Convert the recent_comments block to a view
- #1998664: An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name
- #1957276: Let users set the block instance title for Views blocks in the Block UI
- #2019697: Allow user to customize views block instance machine name
- #2019709: Add an option to machine name elements to increment with a delta instead of failing validation
Comment | File | Size | Author |
---|---|---|---|
#42 | 1998582-42.patch | 7.71 KB | damiankloip |
#42 | interdiff-1998582-42.txt | 519 bytes | damiankloip |
#39 | 1998582-39.patch | 7.69 KB | damiankloip |
#39 | interdiff-1998582-39.txt | 2.97 KB | damiankloip |
#37 | 1998582-37-viewsblock.png | 69.07 KB | saltednut |
Comments
Comment #1
saltednutHere is our ugly little form alter for your review.
Comment #2
saltednutMy apologies, in my haste I've included code in the previous patch from another issue. Here is the proper patch!
Comment #3
dawehnerIt seems to be good to document what the hell we are doing on this lines ...
The codestyle requires $display_id instead of $displayID. I know $name is coming from a views snippet, but let's use $view_id to be consistent with the rest of the code.
Ups missing space.
Comment #4
saltednutComments appended to and added for what we are doing.
I've also addressed the code style issues and removed the trailing whitespace.
Comment #5
damiankloip CreditAttribution: damiankloip commented@brantwynn, This patch is looking quite good. Do you want to add some tests? If not, I will :)
Comment #6
saltednut@damiankloip Yes, if you have the bandwidth to add tests today, go for it. I probably won't be able to get back to it for a few days.
Comment #7
damiankloip CreditAttribution: damiankloip commentedHere are a couple of quick tests. I think if we just test the machine name field is hidden from the markup and that the default value saved for the block id is what we expect, we are ok.
Comment #8
dawehnerJust wondering how we deal with multiple block instances in this patch.
Comment #9
saltednutRegarding #8 - we don't - this should probably be handled by BlockFormController:form() to append an integer to the end of a machine_name if that machine_name already exists. That could be a solution to solve a somewhat related issue: #1998664: An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name
Comment #10
damiankloip CreditAttribution: damiankloip commentedHow about something like this then?
Comment #11
dawehnerUrgs procedural code ... you know just make it a static method on the block plugin ... then we can just unit test that piece.
Comment #12
dawehnerUrgs, I give up now.
Comment #14
damiankloip CreditAttribution: damiankloip commentedLet's not do a unit tests for that, I don't think it's worth it. The web tests assert the created machine names, which tests that logic pretty well.
Comment #15
dawehnerI thought we wanted to stop writing a test ...
Comment #16
damiankloip CreditAttribution: damiankloip commentedI was sure I removed that from the patch!
Comment #17
dawehnerI hated to not have more unit tests like code.
Comment #19
xjmAdded a commit message template to the summary--dawehner, effulgentsia and I did some major pair programming with @brantwynn on the initial patch. :)
Comment #20
damiankloip CreditAttribution: damiankloip commentedThis will fix that last remaining test.
Comment #21
dawehnerGreat, thank you. That's a good step towards a more sane views block.
Comment #22
saltednutThank you!
Comment #24
damiankloip CreditAttribution: damiankloip commented#20: 1998582-20.patch queued for re-testing.
Comment #26
xjm#20: 1998582-20.patch queued for re-testing.
Comment #26.0
xjmUpdated issue summary.
Comment #27
dawehnerThis has been RTBC before.
Comment #28
dawehner#20: 1998582-20.patch queued for re-testing.
Comment #30
dawehnerWhile fixing the bugs ... you could also just use $this->controller->get here.
Comment #31
damiankloip CreditAttribution: damiankloip commentedComment #32
jibranI think we don't need this.
Comment #33
damiankloip CreditAttribution: damiankloip commentedGood spot, we don't need this at all. :)
Comment #34
tim.plunkettThis sucks a lot. Oh well.
This should be $block_plugin = $form_state['controller']->getEntity()->getPlugin();
Comment #35
dawehnerNice!
Comment #36
damiankloip CreditAttribution: damiankloip commentedI guess that was a cross post?! Here is that change too. Tim can you re-RTBC?
Comment #37
saltednutThis looks good. I am able to place a new Views block without entering a machine_name. After placing the block, I am able to see the auto-magic-machine-name.
Comment #38
tim.plunkettSorry, a couple more nitpicks :(
This isn't used by the patch
This is fairly obvious to me, but it could use a code comment.
Implements hook_form_BASE_FORM_ID_alter() for the block form.
Needs ::, and should technically use the fully-qualified class name
Is this really the best way to identify this form?
Missing .
auto-generated
Comment #39
damiankloip CreditAttribution: damiankloip commentedAlas, I'm afraid it looks like it is... :(
Fixed the other points.
Comment #40
saltednutWe're actually implementing hook_form_FORM_ID_alter but I don't know what best practice for commenting around that strange hook is.
Could be worse - could be using hook_form_alter and also looking for the form_id. :)
Unsure if this needs work to fix the comment.
Comment #41
dawehnerThe standard is according to http://drupal.org/node/1354
Comment #42
damiankloip CreditAttribution: damiankloip commentedLet's try and put this issue to bed!
Comment #43
saltednutManually tested this again, for the sake of brevity, everything I said in #37 remains the same.
I also ran the testbot locally for all Blocks and Views tests and did not see any issues.
Comment #44
xjmI filed a couple followups:
Comment #44.0
xjmGet rid of dumb parens.
Comment #45
xjmThis blocks our block conversions.
Comment #46
alexpottCommitted 1a290b5 and pushed to 8.x. Thanks!
Comment #47
damiankloip CreditAttribution: damiankloip commentedGood god - thank you! :)
Comment #48
xjmI made #2019697: Allow user to customize views block instance machine name active.
Comment #49
sdboyer CreditAttribution: sdboyer commentedi like this pattern. we should do it more. a tiddly bit cleaner, though.
so, i wrote another patch: #2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController
Comment #50
sdboyer CreditAttribution: sdboyer commentedugh, sorry, zombie form values.
Comment #51.0
(not verified) CreditAttribution: commentedUpdated issue summary.