Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Warning Undefined array key "VALUE" in Option.php #1934

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

Shadow243
Copy link
Contributor

@Shadow243 Shadow243 commented Oct 14, 2023

Fixing an issue of the "Undefined array key "VALUE" notice in Option.php.
Link to the reported issue: https://app.glitchtip.com/evoludata/issues/2379545

@finwe
Copy link
Member

finwe commented Oct 15, 2023

Thanks for the PR. Though it lacks a test and more thorough explanation of what and how this should solve things. Please elaborate. The link is not enough, mainly because it shows an empty page, at least for me.

@Shadow243
Copy link
Contributor Author

Shadow243 commented Oct 18, 2023

Thanks for your feedback.

Regarding the issue with the provided link, it is currently protected by credentials, making it inaccessible for a complete understanding. To enhance clarity and provide a more comprehensive view of the problem, I have included a screenshot that offers additional context and detailed insights into the issue. This screenshot serves as a valuable supplement to the link and will aid in a more thorough comprehension of the matter at hand.

ERROR - Screenshot 2023-10-18 at 23 50 06

@finwe
Copy link
Member

finwe commented Oct 18, 2023

This does not provide any meaningful additional information at all.

@Shadow243
Copy link
Contributor Author

Shadow243 commented Oct 18, 2023

According to the function:

public function open($attr, &$ahtml, &$ihtml)
{
  $this->mpdf->lastoptionaltag = '';
  $this->mpdf->selectoption['ACTIVE'] = true;
  $this->mpdf->selectoption['currentSEL'] = false;
  if (empty($this->mpdf->selectoption)) {
	  $this->mpdf->selectoption['MAXWIDTH'] = '';
	  $this->mpdf->selectoption['SELECTED'] = '';
  }
  if (isset($attr['SELECTED'])) {
	  $this->mpdf->selectoption['SELECTED'] = '';
	  $this->mpdf->selectoption['currentSEL'] = true;
  }
  if (isset($attr['VALUE'])) {
	  $attr['VALUE'] = UtfString::strcode2utf($attr['VALUE']);
	  $attr['VALUE'] = $this->mpdf->lesser_entity_decode($attr['VALUE']);
	  if ($this->mpdf->onlyCoreFonts) {
		  $attr['VALUE'] = mb_convert_encoding($attr['VALUE'], $this->mpdf->mb_enc, 'UTF-8');
	  }
  }
  $this->mpdf->selectoption['currentVAL'] = $attr['VALUE'];
}

There is a condition checking if $attr['VALUE'] is set but while assigning $attr['VALUE'] to $this->mpdf->selectoption['currentVAL'] there is no.

We could change it like this: $this->mpdf->selectoption['currentVAL'] = $attr['VALUE'] ?? null; instead of adding it inside the condition. The issue here is that we are trying to access an undefined array key VALUE

@finwe
Copy link
Member

finwe commented Oct 19, 2023

Cool, now add a test to the PR with a code example that is affected by this fix. See test/Issues for examples.

@Shadow243 Shadow243 force-pushed the undefine_arraykey_1 branch 2 times, most recently from cba9110 to d82b1a0 Compare October 20, 2023 22:57
@Shadow243
Copy link
Contributor Author

Missing Test added.

@finwe
Copy link
Member

finwe commented Oct 22, 2023

This way of testing works, I guess, but I still would like a test to present an actual failing HTML snippet.

@Shadow243 Shadow243 force-pushed the undefine_arraykey_1 branch 2 times, most recently from 37b36d3 to 311f280 Compare October 22, 2023 22:08
@Shadow243
Copy link
Contributor Author

Shadow243 commented Oct 22, 2023

Test with failing HTML snippet added too.

The HTML code

<select>
 <option value="this is valid">Option 1</option>
 <option selected>Option 2</option>
</select>

contains an element without the 'value' attribute defined, and this specific structure is the direct cause of the error.

This test should pass without applied correction.

public function testWithFailingHtmlSnippet()
{
   $mpdf = $this->mpdf;

   $html = '<select><option value="this is valid">Option 1</option><option selected>Option 2</option>. </select>';

   try {
	$mpdf->WriteHTML($html);
   } catch (\Throwable $th) {
	$this->assertEquals('Undefined array key "VALUE"', $th->getMessage());
   }
}

I changed also this:

$this->mpdf->selectoption['currentVAL'] = $attr['VALUE'] ?? null; 

to

$this->mpdf->selectoption['currentVAL'] = isset($attr['VALUE']) ? $attr['VALUE'] : null;

to support php version 5.x

@finwe
Copy link
Member

finwe commented Oct 23, 2023

Great. Now that we know what HTML causes this, let's make this even more up to standard:

value
The content of this attribute represents the value to be submitted with the form, should this option be selected. If this attribute is omitted, the value is taken from the text content of the option element.

So, when the value attribute is not set, the textual value of the element should be used. Can we easilly set that instead of a default null?

@Shadow243
Copy link
Contributor Author

Requested updates Done.

With this, we are putting the textual value instead of null:

$this->mpdf->selectoption['currentVAL'] = isset($attr['VALUE']) ? $attr['VALUE'] : $ahtml[$ihtml + 1];

@Shadow243
Copy link
Contributor Author

@finwe I'm waiting for your review to make sure every thing is now okay.

@finwe finwe mentioned this pull request Mar 11, 2024
1 task
@finwe finwe merged commit 7f1b31f into mpdf:development Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants