Zend\Form Should throw exception if try to get() an element that does not exist #4572

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@joshribakoff

I'm one of your ZF1 certified engineers and this documentation is difficult to follow for me. In your 'getting started' guide, you did not call parent::__construct(). This made it so I can't even display my form. I couldn't find the documentation to do a pull request for that part

If you do this in your view:
echo $this->formRow($form->get('field_that_doesnt_exist'));

Instead of a helpful message like "trying to get() a field that doesnt exist", the obscure error message I actually got was:
"Catchable fatal error: Object of class Zend\Form\View\Helper\FormRow could not be converted to string"

This pull request will throw an exception if you try to get() an element that does not exist.

@joshribakoff

All the tests passed on Travis, but Travis still reports failure

@weierophinney
Member

@joshribakoff The manual links to the specific page in the documentation repository -- the left sidebar has both "show source" and "edit source" links (on each of framework.zend.com and rtfd.org). The repository itself is https://github.com/zendframework/zf2-documentation.

As for the failure on 5.3.3, I've retriggered the job to see if it runs differently. One thing we've seen happen in the past with 5.3.3 is that changes that are incompatible with that version occasionally trigger segfaults, which you typically cannot detect from the logged output. If the failure occurs again, I suggest trying to run with 5.3.3, or finding somebody else to run your branch under 5.3.3, to see what the errors are.

@weierophinney weierophinney commented on an outdated diff Jun 1, 2013
library/Zend/Form/Fieldset.php
@@ -10,6 +10,7 @@
namespace Zend\Form;
use Traversable;
+use Zend\Form\Exception\InvalidElementException;
@weierophinney
weierophinney Jun 1, 2013 Zend Framework member

This does not need to be imported; just refer to it as Exception\InvalidElementException. (We only import if the class/namespace can not be resolved relative to the current namespace, or to resolve ambiguous names.)

@weierophinney weierophinney commented on an outdated diff Jun 1, 2013
library/Zend/Form/Fieldset.php
* @param string $elementOrFieldset
* @return ElementInterface
*/
public function get($elementOrFieldset)
{
if (!$this->has($elementOrFieldset)) {
- return null;
+ throw new InvalidElementException("No element by the name of [$elementOrFieldset] found in form.");
@weierophinney
weierophinney Jun 1, 2013 Zend Framework member

Use sprintf instead of string interpolation here. Additionally, do not end exception messages with punctuation (PHP treats it as a sentence fragment when reporting).

@joshribakoff joshribakoff Zend\Form - Exception when try to get() element that doesn't exist sh…
…ould use sprintf, message should not use punctuation & should not import the exception class
4ebdb06
@joshribakoff

Ok I made the requested changes. Out of curiosity what do you mean "PHP displays the message as sentence fragments"? I did a Google search about this but couldn't find any information, and I'd like to learn.

I also ran the tests on my laptop, which uses PHP 5.3.x, but not 5.3.3 specifically. The tests passed there. I'd have to compile from source to test that version specifically. I will attempt that & comment back.

@weierophinney
Member

PHP displays exceptions something like "InvalidArgumentException with
message your message here raised in ..." - that's what I meant by sentence
fragment.

Since the errors only exist on 5.3.3, we need to figure out what exactly is
happening. If you need help, pop into #zftalk.dev on Freenode and ask
anyone there.

On Saturday, June 1, 2013, Josh Ribakoff wrote:

Ok I made the requested changes. Out of curiosity what do you mean "PHP
displays the message as sentence fragments"? I did a Google search about
this but couldn't find any information, and I'd like to learn.

I also ran the tests on my laptop, which uses PHP 5.3.x, but not 5.3.3
specifically. The tests passed there. I'd have to compile from source to
test that version specifically. I will attempt that & comment back.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4572#issuecomment-18799625
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@joshribakoff

I installed php-5.3.3, using phpbrew. I ran the tests for Zend\Form and they all passed. When I run tests for all components, I get some failures in unrelated components. These failures occur even without my changes present. I tried asking about this in IRC but have not received any response as of yet.

$ ../vendor/bin/phpunit ZendTest/Form/
Configuration read from /home/josh/www/joshribakoff/zf2/tests/phpunit.xml.dist

(test progress omitted)

Time: 6 seconds, Memory: 113.50Mb

OK, but incomplete or skipped tests!
Tests: 1751, Assertions: 2642, Skipped: 39.


$ php -v
PHP 5.3.3 (cli)

I did see test failures in ZendTest/Json under 5.3.3, these are not my failures:

$ git log --oneline
4ebdb06 Zend\Form - Exception when try to get() element that doesn't exist should use sprintf, message should not use punctuation & should not import the exception class
0a79999 Zend\Form - trying to get() a non-existent element should raise an exception
9d34a33 Merge branch 'EvanDotPro-hotfix/zend-auth-duplicate-code'

$ git checkout 9d34a33cbae

$ ../vendor/bin/phpunit ZendTest/Json/

Configuration read from /home/josh/www/joshribakoff/zf2/tests/phpunit.xml.dist

S..................................S........S.S.........F......  63 / 228 ( 27%)
............................................................... 126 / 228 ( 55%)
............................................................... 189 / 228 ( 82%)
........I..............................

Time: 1 second, Memory: 17.50Mb

There was 1 failure:

1) ZendTest\Json\JsonTest::testJsonPrettyPrintWorksWithArrayNotationInStringLiteral
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
    "obj":{
+    "__className":"stdClass",
     "test":1,
     "faz":"fubar"
    }
   }
  }
 }

/home/josh/www/joshribakoff/zf2/tests/ZendTest/Json/JsonTest.php:872

FAILURES!
Tests: 228, Assertions: 766, Failures: 1, Incomplete: 1, Skipped: 4.

I also have another failure when running tests, that occurs even without my changes present, when using php-5.3.3:

PHP Catchable fatal error:  Argument 1 passed to Zend\Config\Config::__construct() must be of the type array, none given, called in /home/josh/www/joshribakoff/zf2/tests/ZendTest/Di/DiCompatibilityTest.php on line 64 and defined in /home/josh/www/joshribakoff/zf2/library/Zend/Config/Config.php on line 64
PHP Notice:  Unknown: /tmp/zfcache_dba_51aaa61e61d06.db4: unable to flush: No such file or directory in Unknown on line 0
@weierophinney
Member

So, I've just merged your PR locally, and when I run PHPUnit under 5.3.3, I get a segfault. I generally then run PHPUnit with the --tap switch, as it lets me see what the last test run before failure was... and all tests ran. Running PHPUnit without the --tap switch again gives the segfault.

To the best of my knowledge, the error is happening in ZendTest\View\Helper\FormInput::testAllValidFormMarkupAttributesPresentInElementAreRendered(). This particular method uses a dataProvider, and it seems to be the specific number of items in the data provider that are causing an issue. If I remove any 12 items out of it, leaving only 90, it works fine; it's only when we add even one more to that list that it breaks.

As such, I'm going to break it into two test cases with separate data providers to see if I can get these tests passing.

@weierophinney weierophinney added a commit that referenced this pull request Jun 10, 2013
@weierophinney weierophinney [#4572] Break test case/provider into 2
- PHPUnit under PHP 5.3.3 was throwing segfaults when run with no
  arguments, and yet passing fine when given --tap and other arguments.
  Based on experimentation, it appeared to be solely based on the number
  of items in the data provider. Split the data provider and test case
  into two, and all problems were solved.
bd2c6f4
@weierophinney weierophinney added a commit that referenced this pull request Jun 10, 2013
@weierophinney weierophinney Merge branch 'hotfix/4572' into develop
Forward port #4572
e37ab13
@weierophinney weierophinney added a commit that closed this pull request Jun 10, 2013
@weierophinney weierophinney Merge branch 'hotfix/4572'
Close #4572
9cd25e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment