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

[Yaml] Remove internal arguments from the api #21350

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@GuilhemN
Copy link
Contributor

commented Jan 19, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

Can you rebase to get rid of the merge commit?

@GuilhemN GuilhemN force-pushed the GuilhemN:REMOVEINTERNALARGUMENTS branch from 06706f2 to 847fda8 Jan 19, 2017

@GuilhemN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2017

Sure, sorry.

@xabbuh xabbuh added the Deprecation label Jan 20, 2017

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

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

@xabbuh
Copy link
Member

left a comment

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

$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);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 20, 2017

Member

The constructor arguments [...]

Yaml
----

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 20, 2017

Member

The constructor arguments [...]

----

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 20, 2017

Member

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

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

* Duplicate mapping keys lead to a `ParseException`.

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 20, 2017

Member

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

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

👍 after the changes proposed by @xabbuh

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

This comment has been minimized.

Copy link
@Taluu

Taluu Jan 20, 2017

Contributor

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)

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 20, 2017

Author Contributor

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 GuilhemN force-pushed the GuilhemN:REMOVEINTERNALARGUMENTS branch from 847fda8 to e176c09 Jan 20, 2017

@GuilhemN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2017

@xabbuh I fixed your comments.

@xabbuh

xabbuh approved these changes Jan 21, 2017

* @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())

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 21, 2017

Member

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

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 21, 2017

Author Contributor

👍, i forgot to revert it.

@xabbuh

xabbuh approved these changes Jan 21, 2017

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jan 21, 2017

Thank you @GuilhemN.

@xabbuh xabbuh closed this in 0abe862 Jan 21, 2017

@GuilhemN GuilhemN deleted the GuilhemN:REMOVEINTERNALARGUMENTS branch Jan 21, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.