Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper

"Having a crack" as they say here.

kim.pepper’s picture

Status: Active » Needs review
FileSize
3.88 KB

First shot at the conversion.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, 1946444-convert-path-confirm-iface-2.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +FormInterface, +WSCCI-conversion

The last submitted patch, 1946444-convert-path-confirm-iface-2.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
4.58 KB

Wasn't loading the path alias correctly, which I'm now doing by injecting a Path crud object on the form and calling path->load().

Thanks to @timplunkett for the IRC help.

Status: Needs review » Needs work

The last submitted patch, 1946444-convert-path-confirm-iface-6.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/path/lib/Drupal/path/Form/DeleteForm.phpundefined
@@ -0,0 +1,91 @@
+  public function buildForm(array $form, array &$form_state, int $pid = NULL) {

You can't typehint with 'int', especially since it could be an int or string.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
969 bytes
4.58 KB

Removed type hinting per #8 and fixed some docs.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

catch’s picture

Status: Reviewed & tested by the community » Needs review

hmm shouldn't the cancel path be a route rather than a path?

catch’s picture

Status: Needs review » Fixed

Opened #1963394: ConfirmFormBase::getCancelPath() should allow for a route since this is a bit of a can of worms. Committed/pushed to 8.x.

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