Documentation fixes #243

BenMorel · 2013-01-07T13:40:03Z

Carrying on with the work done on the ORM, here are my documentation fixes for the DBAL library:

Missing docblocks
Missing / incorrect @param / @return / @throws annotations
Missing newlines between annotations
Incorrect vertical alignment of @param annotations

In addition:

Removal of return statements on void methods
Cleanup of unused private / local variables

Ocramius · 2013-01-07T13:42:14Z

@BenMorel rebasing all those will be hell for people with currently open PRs (and for you too if merging takes some time).

Please consider squashing all those once you got it completed...

BenMorel · 2013-01-07T13:44:00Z

@Ocramius it is complete!

Ocramius · 2013-01-07T14:12:18Z

Please squash then :S

BenMorel · 2013-01-07T16:44:05Z

Squashed :-)

BenMorel · 2013-01-07T16:49:53Z

Oops, I just noticed that some files are shown as entirely replaced, instead of just showing the diff.
I have no idea why, as the others diffs appear correctly.

@Ocramius Any idea how to fix this?

Ocramius · 2013-01-07T16:53:29Z

@BenMorel that's just GIT trying to outsmart things.

Anyway, I just noticed you kept lgpl everywhere. Can you move to MIT? The entire project migrated some months ago :)

https://github.com/doctrine/dbal/blob/master/composer.json#L7

BenMorel · 2013-01-07T17:04:51Z

I think I got it: I must have overwritten a file in some way, and GIT has detected a timestamp change, hence considering that the file has been replaced. I think I will be able to fix that.

No problem to include the MIT license, can you point to a sample file where it's included already for reference?

Ocramius · 2013-01-07T17:07:12Z

https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L2-L18

And remove it from the class docs :)

BenMorel · 2013-01-07T18:06:21Z

Thanks, I will do that.
I managed to fix the file "replace" problem, it actually turned out that something had converted several files to Windows line endings. I converted back all the changed files to Linux line endings, and everything is back to normal.

stof · 2013-01-07T22:07:36Z

lib/Doctrine/DBAL/ConnectionException.php

- * @author      Jonathan H. Wage <[email protected]
+ * @link    www.doctrine-project.org
+ * @since   2.0
+ * @version $Revision: 4628 $


Please remove this $Revision stuff which is a left-over from some SVN times (just like you removed it in the license header)

BenMorel · 2013-01-08T11:37:23Z

@Ocramius The license has been updated to MIT. This is already squashed in the previous commit.

@stof The SVN leftovers have been removed, and the links updated.
I've done a separate commit for you to review the changes.
If needed, I can squash it in the previous commit as well!

guilhermeblanco · 2013-01-08T15:57:24Z

lib/Doctrine/DBAL/Statement.php

    public function setFetchMode($fetchMode, $arg2 = null, $arg3 = null)
    {
        if ($arg2 === null) {
-            return $this->stmt->setFetchMode($fetchMode);
+            $this->stmt->setFetchMode($fetchMode);


Any reason why you removed the return?
PDOStatement->setFetchMode returns a mixed value. According to docs, it returns 1 in case of success or false on failure.

Ok, then I need to change the interface and all classes implementing it.
The interface did not define a return type, so I assumed void.

And most of the implementing classes where returning void:

Cache\ArrayStatement

Cache\ResultCacheStatement

Driver\IBMDB2\DB2Statement

Driver\Mysqli\MysqliStatement

Portability\Statement

...

Actually, it does not return mixed, it returns bool.
This is an error in the PHP documentation (I will file it there):

$pdo = new PDO('sqlite::memory:'); $st = $pdo->query('SELECT * FROM sqlite_master'); var_export($st->setFetchMode(PDO::FETCH_NUM)); // true, not 1

FYI, the online documentation has been fixed since.

guilhermeblanco · 2013-01-08T16:42:28Z

Patch looks great, although some changes I highlighted are still required.
As soon as you fix the pointed elements, I'm fine to merge.

BenMorel · 2013-01-08T16:49:08Z

@guilhermeblanco Thanks for your comments. I will have a look at them tonight.

BenMorel · 2013-01-11T16:03:56Z

@guilhermeblanco Could you please comment on this and this? Thanks!

guilhermeblanco · 2013-01-11T22:53:40Z

I'm fine with the setFetchMode change

BenMorel · 2013-01-12T12:28:46Z

Ok, we're good then! Do you want me to squash all the extra commits into the first one?

Ocramius · 2013-01-12T18:57:23Z

@BenMorel that's always useful =)

BenMorel · 2013-01-12T21:57:18Z

Squashed!

BenMorel · 2013-01-27T10:24:17Z

I'm done with this job, do let me know if you have any comment!

guilhermeblanco · 2013-01-27T17:33:49Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -1559,8 +1634,8 @@ protected function onSchemaAlterTableRemoveColumn(Column $column, TableDiff $dif

    /**
     * @param ColumnDiff $columnDiff
-     * @param TableDiff $diff
-     * @param array $columnSql
+     * @param TableDiff  $diff


FQCN here and next line

BenMorel · 2013-01-29T20:00:53Z

@guilhermeblanco

I replaced short class names with their FQCN
I replaced the primitive type abbreviations with their full name
I added the missing line breaks before the return statements

GitHub keeps displaying some of the comments above for some reason, but they have been updated as you can see by clicking on view full changes.

BenMorel · 2013-02-09T16:48:59Z

Any other comments on this PR?
It would be nice to merge before it gets too much out of sync with master!

BenMorel · 2013-06-11T15:53:14Z

Any update on this PR? I can rebase it on the current master if everything is OK.

beberlei · 2013-06-21T12:32:07Z

rebase pls

BenMorel · 2013-06-21T19:16:00Z

@beberlei Rebased!

beberlei · 2013-06-21T19:25:33Z

@BenMorel still not mergable from the UI :(

- Fixed docblocks - Removed return statements on void methods - Cleaned up unused private / local variables - Fixed license issues - Removed SVN leftovers - Fixed doctrine-project.org links - Added line breaks before return statements

BenMorel · 2013-06-21T22:24:26Z

@beberlei Looks like there's been a new commit in the meantime. Rebased again!

Documentation fixes

stof reviewed Jan 7, 2013
View reviewed changes

guilhermeblanco reviewed Jan 8, 2013
View reviewed changes

guilhermeblanco reviewed Jan 27, 2013
View reviewed changes

BenMorel mentioned this pull request Apr 5, 2013

Documentation fixes doctrine/common#245

Merged

Fixed documentation.

f0eaaf6

- Fixed docblocks - Removed return statements on void methods - Cleaned up unused private / local variables - Fixed license issues - Removed SVN leftovers - Fixed doctrine-project.org links - Added line breaks before return statements

beberlei added a commit that referenced this pull request Jun 21, 2013

Merge pull request #243 from BenMorel/master

270cd8b

Documentation fixes

beberlei merged commit 270cd8b into doctrine:master Jun 21, 2013

doctrinebot mentioned this pull request Dec 6, 2015

DCOM-163: [GH-245] Documentation fixes doctrine/common#464

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation fixes #243

Documentation fixes #243

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

stof Jan 7, 2013

BenMorel commented Jan 8, 2013

guilhermeblanco Jan 8, 2013

BenMorel Jan 8, 2013

BenMorel Jan 8, 2013

BenMorel Jan 12, 2013

guilhermeblanco commented Jan 8, 2013

BenMorel commented Jan 8, 2013

BenMorel commented Jan 11, 2013

guilhermeblanco commented Jan 11, 2013

BenMorel commented Jan 12, 2013

Ocramius commented Jan 12, 2013

BenMorel commented Jan 12, 2013

BenMorel commented Jan 27, 2013

guilhermeblanco Jan 27, 2013

BenMorel commented Jan 29, 2013

BenMorel commented Feb 9, 2013

BenMorel commented Jun 11, 2013

beberlei commented Jun 21, 2013

BenMorel commented Jun 21, 2013

beberlei commented Jun 21, 2013

BenMorel commented Jun 21, 2013

Documentation fixes #243

Documentation fixes #243

Conversation

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

Ocramius commented Jan 7, 2013

BenMorel commented Jan 7, 2013

stof Jan 7, 2013

Choose a reason for hiding this comment

BenMorel commented Jan 8, 2013

guilhermeblanco Jan 8, 2013

Choose a reason for hiding this comment

BenMorel Jan 8, 2013

Choose a reason for hiding this comment

BenMorel Jan 8, 2013

Choose a reason for hiding this comment

BenMorel Jan 12, 2013

Choose a reason for hiding this comment

guilhermeblanco commented Jan 8, 2013

BenMorel commented Jan 8, 2013

BenMorel commented Jan 11, 2013

guilhermeblanco commented Jan 11, 2013

BenMorel commented Jan 12, 2013

Ocramius commented Jan 12, 2013

BenMorel commented Jan 12, 2013

BenMorel commented Jan 27, 2013

guilhermeblanco Jan 27, 2013

Choose a reason for hiding this comment

BenMorel commented Jan 29, 2013

BenMorel commented Feb 9, 2013

BenMorel commented Jun 11, 2013

beberlei commented Jun 21, 2013

BenMorel commented Jun 21, 2013

beberlei commented Jun 21, 2013

BenMorel commented Jun 21, 2013