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.
Comment | File | Size | Author |
---|---|---|---|
#41 | custom-block-1978166-41.patch | 2.48 KB | tim.plunkett |
#38 | custom-block-1978166-38.patch | 8.38 KB | effulgentsia |
#36 | custom-block-1978166-36.patch | 8.42 KB | tim.plunkett |
#34 | block-add-type-1978166.33.patch | 7.54 KB | larowlan |
#29 | block-add-type-1978166.28.patch | 6.48 KB | smiletrl |
Comments
Comment #1
larowlanPostponed on #1978168: Convert block/add to Controller
Comment #2
larowlanComment #3
larowlanComment #4
ParisLiakos CreditAttribution: ParisLiakos commentedmaybe add a todo over this to #1981644: Figure out how to deal with 'title/title callback' ?
Besides this looks good
Comment #5
alexpottNow that #1913618: Convert EntityFormControllerInterface to extend FormInterface has gone in shouldn't we be using _entity_form in the route definition for this?
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commented_entity_form: custom_block.add
? i am not sure what happens here, cause we need the have the dummy new entity with the type assigned for this to work..but i would lie, havent tried that yetComment #7
larowlanAs discussed in irc, we can't use _entity_form in its current form because we have a dynamic type. So even if we could change it to custom_block.default.basic to nominate the bundle, we'd be hard-coding the bundle when we need to take that from the slug.
Fixes request at #4
Comment #8
larowlanDamn it, wrong patch
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedgreat, thanks! i think its ready to go then, if alex does not object..but Crell also agreed in WSSCI's meeting today that we should better use drupal_set_title there *temporarily* instead of another callbacks
Comment #10
andypostThis will need re-roll after #1982984: Create Drupal::entityManager for improved DX and #1982980: Move entity_get_form to Drupal\Core\Entity\EntityManager::getForm()
Comment #11
webchickThanks guys! Regarding #7:
I think we should instead fix _entity_form to allow for dynamic types, because we want to make sure to get these route definitions correct the first time. If this pattern gets introduced here, it's going to be copy/pasted into other similar forms, so let's set a good example.
After that, we can change this hunk to use _entity_form instead of _content:
Comment #12
larowlanSo this adds a _values option to the defaults in the yml that hands _entity_form a callable for initializing the entity object's default values.
Works for me™
Comment #13
larowlandon't need refs to $form and $form_state here c/p fail
Comment #14
larowlanwhoops, yes it does -review fail
Comment #15
larowlanSo this removes the need for reflection.
The _values function gets passed the request instead - and can get the attributes it needs.
Comment #16
larowlanAnd this one gets the _values from $request->attributes which is already in scope, removing the need to duplicate much of HtmlFormController::content.
Comment #17
tim.plunkettThis looks pretty awesome.
Where is Response used? or is this left over from an earlier patch.
No extra line needed
I think these comments should be combined, and the if() should be on one line
There is now a \Drupal\custom_block\CustomBlockTypeInterface that should be used to typehint instead
Maybe its just not clear from the patch context, but why is this a separate method?
This will seem much less hacky when #1982980: Move entity_get_form to Drupal\Core\Entity\EntityManager::getForm() goes in
This should eventually be a check for $this->operation, I switched all the calls around in another issue
Comment #18
larowlanFixes issues identified at #17
Comment #19
larowlanmeh wrong use statement
Comment #20
tim.plunkettThis looks about right. Matches much of our other controller handlers.
This looks wrong. Shouldn't it be Drupal\custom_block\CustomBlockTypeInterface?
I don't get this bit. Should this have been handled elsewhere?
This should ideally be checking $this->operation == 'add', but we don't have that yet.
Comment #21
tim.plunkettMy review was for the wrong patch.
The last two bits are actually cleaned up in #1983682: Convert applicable custom_block_menu() entries to use _entity_form in routing.yml, so this on its own is great.
@larowlan, can we get an improved issue title to imply the new scope of the issue?
Comment #22
larowlanComment #23
alexpottWe need to through an exception here is the request has a _values and it does not contain a
::
Comment #24
Crell CreditAttribution: Crell commentedlarowlan, Tim Plunkett, Alex Pott, and I discussed this at the Extended Sprints. The general consensus was "we don't have enough time before freeze to figure out how to do dynamic default values well, so it's better to not do them than to do them badly."
The _values implementation above feels icky, and is duplicating code from elsewhere (ie, ControllerResolver), which suggests we'd want to us that soon enough, which ends up just getting weird. So let's just not do that. We'll go back to having a trivial wrapper method, which is no worse than the page callbacks that were just trivial wrappers around drupal_get_form() rather than using drupal_get_form() as a page callback. There's no DX regression in doing so.
So, back to the original plan. larowlan, can you provide an updated patch for testbot?
Comment #25
larowlanback to old title.
would you believe the patch at 8 still applies, wonder what bot says
Comment #26
smiletrl CreditAttribution: smiletrl commentedMissing converters in routing file:) Also need tests.
Comment #27
andypostthere's no conversion!
Comment #28
andypostThere's tests for that
tested manually #25 and it works
needs \
not used
do we really need theme here?
Comment #29
smiletrl CreditAttribution: smiletrl commentedMy fault -- I didn't clear cache after I applied patch at #25.
And apparently, test has covered.
Rerolled according to #28
Comment #30
smiletrl CreditAttribution: smiletrl commentedtheme is probably needed here.
Comment #31
dawehnerNice! This is RTBC once it's green.
Comment #32
dawehnerComment #33
alexpottNeeds a reroll
Comment #34
larowlanRe-roll
Comment #36
tim.plunkettThat last reroll had the defaults() stuff in it, I think it was the wrong patch.
This cleans up the CustomBlockController to match the more recent conversions.
Comment #37
larowlanProvided bot agrees
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedHEAD moves fast. I was about to remove the "Needs reroll" tag, but turned out #36 needed yet another reroll. This is a straight reroll, so leaving at RTBC.
Comment #39
alexpottCommitted 6f25f15 and pushed to 8.x. Thanks!
Comment #40
xjmThe tests from this are currently breaking head:
git revert 6f25f1588523
Comment #41
tim.plunkettLet's just fix the fail. It was an overly specific typehint, and the class got removed.
Comment #42
webchickCommitted and pushed to 8.x. Thanks!