[Yaml] Remove internal arguments from the api #21350

GuilhemN · 2017-01-19T21:13:58Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#21230
License	MIT
Doc PR

Reopening of #21230 because of @xabbuh's comment.

This PR removes internal constructor arguments of the Parser class.
This should break nothing as they are only used to build errors message and are clearly meant for internal uses.

This would allow to have a nicer api for #21194 (comment).

fabpot · 2017-01-19T21:34:09Z

Can you rebase to get rid of the merge commit?

GuilhemN · 2017-01-19T21:41:26Z

Sure, sorry.

xabbuh · 2017-01-20T06:58:22Z

FYI, I have updated the "Deprecations?" row of the PR description to match the actual changes.

xabbuh

I just left some minor comments about the wording of the deprecation message. Otherwise, the changes look good to me.

xabbuh · 2017-01-20T07:00:54Z

src/Symfony/Component/Yaml/Parser.php

-        $this->totalNumberOfLines = $totalNumberOfLines;
-        $this->skippedLineNumbers = $skippedLineNumbers;
+        if (func_num_args() > 0) {
+            @trigger_error(sprintf('Constructor arguments $offset, $totalNumberOfLines, $skippedLineNumbers of %s are deprecated and will be removed in 4.0', self::class), E_USER_DEPRECATED);


The constructor arguments [...]

xabbuh · 2017-01-20T07:01:19Z

UPGRADE-3.3.md

+Yaml
+----
+
+ * Constructor arguments `$offset`, `$totalNumberOfLines` and


The constructor arguments [...]

xabbuh · 2017-01-20T07:01:50Z

UPGRADE-3.3.md

+----
+
+ * Constructor arguments `$offset`, `$totalNumberOfLines` and
+   `$skippedLineNumbers` of `Parser` are deprecated and will be


[...] of the Parser class [...]

xabbuh · 2017-01-20T07:02:23Z

UPGRADE-4.0.md

@@ -391,6 +391,9 @@ Yaml

 * Duplicate mapping keys lead to a `ParseException`.

+ * Constructor arguments `$offset`, `$totalNumberOfLines` and


I would make the she changes as in the other upgrade file here too.

javiereguiluz · 2017-01-20T08:28:06Z

👍 after the changes proposed by @xabbuh

Taluu · 2017-01-20T09:05:38Z

src/Symfony/Component/Yaml/Parser.php

-        $this->offset = $offset;
-        $this->totalNumberOfLines = $totalNumberOfLines;
-        $this->skippedLineNumbers = $skippedLineNumbers;
+        if (func_num_args() > 0) {


instead of having to do a if func > 0, if func_args > 1, ..., you can leave the optionnal parameters in the constructor (just undocument those) and just check if func_num_args > 0 to trigger the deprecation :

<?php $f = function ($a = null, $b = null, array $c = array()) { var_dump(func_num_args()); if (func_num_args() > 0) { trigger(); } // ... } $f(); // output 0, no deprecation $f(1); // output 1, deprecation // and so on

It's cleaner IMO (and less conditionnals too)

But then they'll be automatically inserted by IDEs. Imo it is better to remove them to ensure people won't use deprecated arguments and only know it by executing the code.

GuilhemN · 2017-01-20T18:19:17Z

@xabbuh I fixed your comments.

xabbuh · 2017-01-21T07:32:47Z

src/Symfony/Component/Yaml/Parser.php

-     * @param int      $offset             The offset of YAML document (used for line numbers in error messages)
-     * @param int|null $totalNumberOfLines The overall number of lines being parsed
-     * @param int[]    $skippedLineNumbers Number of comment lines that have been skipped by the parser
-     */
    public function __construct($offset = 0, $totalNumberOfLines = null, array $skippedLineNumbers = array())


If we use func_get_arg(), we should remove the arguments here.

👍, i forgot to revert it.

xabbuh · 2017-01-21T07:50:34Z

Thank you @GuilhemN.

carsonbot added Status: Needs Review Yaml labels Jan 19, 2017

GuilhemN force-pushed the REMOVEINTERNALARGUMENTS branch from 06706f2 to 847fda8 Compare January 19, 2017 21:39

xabbuh added the Deprecation label Jan 20, 2017

xabbuh requested changes Jan 20, 2017

View reviewed changes

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Jan 20, 2017

Taluu reviewed Jan 20, 2017

View reviewed changes

[Yaml] Remove internal arguments from the api

e176c09

GuilhemN force-pushed the REMOVEINTERNALARGUMENTS branch from 847fda8 to e176c09 Compare January 20, 2017 18:18

xabbuh approved these changes Jan 21, 2017

View reviewed changes

xabbuh requested changes Jan 21, 2017

View reviewed changes

Remove constructor arguments

1a4626d

xabbuh approved these changes Jan 21, 2017

View reviewed changes

xabbuh closed this in 0abe862 Jan 21, 2017

GuilhemN deleted the REMOVEINTERNALARGUMENTS branch January 21, 2017 07:51

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yaml] Remove internal arguments from the api #21350

[Yaml] Remove internal arguments from the api #21350

GuilhemN commented Jan 19, 2017 •

edited by xabbuh

Loading

fabpot commented Jan 19, 2017

GuilhemN commented Jan 19, 2017

xabbuh commented Jan 20, 2017

xabbuh left a comment

xabbuh Jan 20, 2017

xabbuh Jan 20, 2017

xabbuh Jan 20, 2017

xabbuh Jan 20, 2017

javiereguiluz commented Jan 20, 2017

Taluu Jan 20, 2017 •

edited

Loading

GuilhemN Jan 20, 2017

GuilhemN commented Jan 20, 2017

xabbuh Jan 21, 2017

GuilhemN Jan 21, 2017

xabbuh commented Jan 21, 2017

		@@ -391,6 +391,9 @@ Yaml

		* Duplicate mapping keys lead to a `ParseException`.

		* Constructor arguments `$offset`, `$totalNumberOfLines` and

[Yaml] Remove internal arguments from the api #21350

[Yaml] Remove internal arguments from the api #21350

Conversation

GuilhemN commented Jan 19, 2017 • edited by xabbuh Loading

fabpot commented Jan 19, 2017

GuilhemN commented Jan 19, 2017

xabbuh commented Jan 20, 2017

xabbuh left a comment

Choose a reason for hiding this comment

xabbuh Jan 20, 2017

Choose a reason for hiding this comment

xabbuh Jan 20, 2017

Choose a reason for hiding this comment

xabbuh Jan 20, 2017

Choose a reason for hiding this comment

xabbuh Jan 20, 2017

Choose a reason for hiding this comment

javiereguiluz commented Jan 20, 2017

Taluu Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

GuilhemN Jan 20, 2017

Choose a reason for hiding this comment

GuilhemN commented Jan 20, 2017

xabbuh Jan 21, 2017

Choose a reason for hiding this comment

GuilhemN Jan 21, 2017

Choose a reason for hiding this comment

xabbuh commented Jan 21, 2017

GuilhemN commented Jan 19, 2017 •

edited by xabbuh

Loading

Taluu Jan 20, 2017 •

edited

Loading