Add a maxLength rule for text columns by dmromanov · Pull Request #335 · cakephp/bake

dmromanov · 2017-05-16T22:43:16Z

#284
This pull-request depends on cakephp/cakephp/issues/11295 .

codecov-io · 2017-05-16T23:02:45Z

Codecov Report

Merging #335 into master will increase coverage by 0.07%.
The diff coverage is 82.05%.

@@             Coverage Diff              @@
##             master     #335      +/-   ##
============================================
+ Coverage     92.12%   92.19%   +0.07%     
- Complexity      664      672       +8     
============================================
  Files            25       25              
  Lines          2184     2205      +21     
============================================
+ Hits           2012     2033      +21     
  Misses          172      172

Impacted Files	Coverage Δ	Complexity Δ
src/View/Helper/BakeHelper.php	`100% <100%> (ø)`	`65 <2> (+6)`	⬆️
src/Shell/Task/ModelTask.php	`95.02% <69.56%> (+0.06%)`	`156 <0> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c3c512...f60967d. Read the comment docs.

markstory

Looking good, just wondering if we should fix the possible args issue now.

markstory · 2017-05-17T00:39:10Z

src/Template/Bake/Model/table.ctp

+        if ($rule['rule'] && !isset($rule['provider']) && !isset($rule['args'])):
            $validationMethods[] = sprintf("->%s('%s')", $rule['rule'], $field);
+        elseif ($rule['rule'] && !isset($rule['provider'])):
+            $validationMethods[] = sprintf("->%s('%s', %s)", $rule['rule'], $field, implode(', ', $rule['args']));


Won't this have issues if the arguments are strings?

dmromanov · 2017-05-18T18:32:14Z

@markstory String values are now wrapped with quotes.

Tests are failing because each DBMS has different length limits for string-type fields.

markstory · 2017-05-19T01:47:55Z

src/View/Helper/BakeHelper.php

+    public function escapeArguments($args)
+    {
+        return array_map(function ($v) {
+            if (!is_numeric($v)) {


You should probably escape ' inside strings 😄

markstory · 2017-05-19T01:48:36Z

Tests will need to be passing before we can get this merged.

dmromanov · 2017-05-22T17:47:50Z

Ok, I'll change the code.

ADmad · 2017-07-01T15:28:51Z

Tests still failing.

dmromanov · 2017-10-07T12:31:07Z

My changes fails to pass validation on some DBMS, because of some inconsistency between them. E.g. in SQLite, text fields can have infinite length, while in MySQL, it's not.

To work around this, I am thinking about two possible approaches:

To mock a table schema and build it manually in tests, where a table schema is required.
Make DBMS-specific tests, that would be marked as skipped, if they are run under different DMBS.

@ADmad @markstory Which one is better? Or maybe you can suggest a better solutions?

markstory · 2017-10-07T16:23:26Z

Could we test this validator against varchar columns instead of text ones? Those should have more stable lengths across different db vendors.

dmromanov · 2017-10-07T19:19:08Z

Depends on cakephp/cakephp/issues/11295

markstory · 2017-10-07T20:51:26Z

tests/Fixture/NumberTreesFixture.php

+ *
+ * Generates a tree of data for use testing the tree behavior
+ */
+class NumberTreesFixture extends TestFixture


Why do we need this fixture?

Overall it's a copy of CakePHP's core NumberTreesFixture, but with a change: "name" column now has a length attribute. Since the length attribute is not needed in CakePHP core, and is not covered with tests, it is a subject to change in the future. So I decided to copy it into the repository.
This fixture is used in ModelTaskTest.php::testGetValidationTree().

I didn't see where the fixture was used. My mistake.

dmromanov · 2017-11-23T12:41:34Z

Now with TWIG support.

dmromanov force-pushed the feature/text-maxlength-validator branch from c3c128d to c50a2e6 Compare May 16, 2017 23:02

markstory added this to the 1.3.x milestone May 17, 2017

markstory reviewed May 17, 2017

View reviewed changes

dmromanov force-pushed the feature/text-maxlength-validator branch 3 times, most recently from b94cf73 to 208a4ca Compare May 18, 2017 18:26

markstory reviewed May 19, 2017

View reviewed changes

dmromanov force-pushed the feature/text-maxlength-validator branch 3 times, most recently from bb3761c to 68347e8 Compare May 19, 2017 11:28

dmromanov force-pushed the feature/text-maxlength-validator branch 2 times, most recently from e1c5e08 to 68347e8 Compare June 5, 2017 09:28

dmromanov force-pushed the feature/text-maxlength-validator branch from 68347e8 to afe3e12 Compare June 28, 2017 21:49

dmromanov force-pushed the feature/text-maxlength-validator branch 3 times, most recently from c5e35b7 to 7b6b8f6 Compare October 6, 2017 16:10

dmromanov force-pushed the feature/text-maxlength-validator branch 4 times, most recently from 7d92a67 to d315b22 Compare October 7, 2017 17:24

dmromanov mentioned this pull request Oct 7, 2017

SqlserverSchema return colum length double the actual size cakephp/cakephp#11295

Closed

3 tasks

dmromanov force-pushed the feature/text-maxlength-validator branch from d315b22 to 900f52c Compare October 7, 2017 19:27

markstory reviewed Oct 7, 2017

View reviewed changes

dmromanov force-pushed the feature/text-maxlength-validator branch 3 times, most recently from 7719309 to 8675cf5 Compare October 9, 2017 12:13

markstory modified the milestones: 1.3.x, 1.4.x Oct 20, 2017

dmromanov force-pushed the feature/text-maxlength-validator branch from 8675cf5 to f10333c Compare October 31, 2017 11:12

markstory modified the milestones: 1.4.x, 1.5.x Nov 22, 2017

Added maxLength() validation rule for text columns when baking tables

f60967d

dmromanov force-pushed the feature/text-maxlength-validator branch from f10333c to f60967d Compare November 23, 2017 12:34

markstory merged commit 09de285 into cakephp:master Nov 24, 2017

dmromanov deleted the feature/text-maxlength-validator branch August 19, 2018 22:35

Conversation

dmromanov commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

markstory May 17, 2017

Choose a reason for hiding this comment

Uh oh!

dmromanov commented May 18, 2017

Uh oh!

markstory May 19, 2017

Choose a reason for hiding this comment

Uh oh!

markstory commented May 19, 2017

Uh oh!

dmromanov commented May 22, 2017

Uh oh!

ADmad commented Jul 1, 2017

Uh oh!

dmromanov commented Oct 7, 2017

Uh oh!

markstory commented Oct 7, 2017

Uh oh!

dmromanov commented Oct 7, 2017

Uh oh!

markstory Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

dmromanov Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory Oct 8, 2017

Choose a reason for hiding this comment

Uh oh!

dmromanov commented Nov 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmromanov commented May 16, 2017 •

edited

Loading

codecov-io commented May 16, 2017 •

edited

Loading

dmromanov Oct 7, 2017 •

edited

Loading