Problem/Motivation

When creating a new front-end environment there is no validation of the maxlength of the machine name

Steps to reproduce

Add a new environment
Edit the machine name
Make it longer than 32 chars
See the site error out

Proposed resolution

Add max length
Add tests

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 added-maxlength-and-test-3205765-14.patch2.29 KBanjali rathod
#11 added-maxlength-test-3205765-10.patch2.31 KBanjali rathod
#9 added-maxlength-test-3205765-9.patch2.01 KBanjali rathod
#5 added-maxlength-3205765-5.patch2 KBanjali rathod

Comments

larowlan created an issue. See original summary.

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: Unassigned » anjali rathod
anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented 
$form['id'] = [
      '#type' => 'machine_name',
      '#default_value' => $entity->id(),
      '#maxlength' => 32,
      '#machine_name' => [
        'exists' => '\Drupal\build_hooks\Entity\FrontendEnvironment::load',
      ],
      '#disabled' => !$entity->isNew(),
    ];

Is test needed for the above changes?

$longMachineName = mb_strtolower($this->randomMachineName(34));
    $this->submitForm([
      'id' => $longMachineName,
      'label' => $longMachineName,
      'url' => 'http://example.com/' . $longMachineName,
      'deployment_strategy' => TriggerInterface::DEPLOYMENT_STRATEGY_MANUAL,
      'settings[whiz]' => $longMachineName,
    ], 'Save');
    $assert = $this->assertSession();
    $assert->statusCodeEquals(500);

For test I have done this.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Yes please, there's an existing test for the config form.

See \Drupal\Tests\build_hooks\Functional\UiTest::assertThatAdminCanAddFrontEndEnvironment there's an existing test that does this:


// Try to submit the form with whiz length 2 characters.
    $this->submitForm([
      'id' => $random,
      'label' => $random,
      'url' => 'http://example.com/' . $random,
      'deployment_strategy' => TriggerInterface::DEPLOYMENT_STRATEGY_MANUAL,
      'settings[whiz]' => $whiz,
    ], 'Save');
    $assert = $this->assertSession();
    $assert->pageTextContains('Whiz must contains minimum 3 characters.');

if it were changed to this

// Try to submit the form with whiz length 2 characters and a long ID.
    $this->submitForm([
      'id' => mb_strtolower($this->randomMachineName(34)),
      'label' => $random,
      'url' => 'http://example.com/' . $random,
      'deployment_strategy' => TriggerInterface::DEPLOYMENT_STRATEGY_MANUAL,
      'settings[whiz]' => $whiz,
    ], 'Save');
    $assert = $this->assertSession();
    $assert->pageTextContains('Whiz must contains minimum 3 characters.');
$assert->pageTextContains('Id must be no longer than 32 characters.'); // Update this to the real message.

that would be sufficient.

And then if we could use the constant \Drupal\Core\Entity\EntityTypeInterface::BUNDLE_MAX_LENGTH instead of hard-coding 32

Thanks!

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: anjali rathod » Unassigned
Status: Active » Needs review
StatusFileSize
new added-maxlength-3205765-5.patch2 KB

Thanks , made the changes as mentioned.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Assuming this comes back green - thank you 💪

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: added-maxlength-3205765-5.patch, failed testing. View results

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: Unassigned » anjali rathod

The test failed. checking for the text

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new added-maxlength-test-3205765-9.patch2.01 KB

Status: Needs review » Needs work

The last submitted patch, 9: added-maxlength-test-3205765-9.patch, failed testing. View results

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: anjali rathod » Unassigned
Status: Needs work » Needs review
StatusFileSize
new added-maxlength-test-3205765-10.patch2.31 KB

Status: Needs review » Needs work

The last submitted patch, 11: added-maxlength-test-3205765-10.patch, failed testing. View results

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: Unassigned » anjali rathod
anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Assigned: anjali rathod » Unassigned
Status: Needs work » Needs review
StatusFileSize
new added-maxlength-and-test-3205765-14.patch2.29 KB

@larowlan hopefully this patch should pass. Tried testing for the first time hence there was a lot of confusion about the error message.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

awesome work @Anjali Rathod, thanks for sticking at it

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

This will go out as 3.1.3 shortly

Status: Fixed » Closed (fixed)

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