NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Yaml] Remove internal arguments from the api#21350

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:REMOVEINTERNALARGUMENTS

Closed

[Yaml] Remove internal arguments from the api#21350

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:REMOVEINTERNALARGUMENTS

Conversation

Copy link

Contributor

GuilhemN commentedJan 19, 2017•
edited by xabbuh
Loading

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 theParser 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).

carsonbot added Status: Needs Review Yaml labels

Jan 19, 2017

Copy link

Member

fabpot commentedJan 19, 2017

Can you rebase to get rid of the merge commit?

GuilhemN force-pushed theREMOVEINTERNALARGUMENTS branch from06706f2 to847fda8Compare

January 19, 2017 21:39

Copy link

ContributorAuthor

GuilhemN commentedJan 19, 2017

Sure, sorry.

xabbuh added the Deprecation label

Jan 20, 2017

Copy link

Member

xabbuh commentedJan 20, 2017

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

xabbuh requested changes

Jan 20, 2017

View reviewed changes

Copy link

Member

xabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/Symfony/Component/Yaml/Parser.php Outdated

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The constructor arguments [...]

UPGRADE-3.3.md Outdated

		Yaml
		----

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The constructor arguments [...]

UPGRADE-3.3.md Outdated

		----

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[...] of theParser class [...]

UPGRADE-4.0.md Outdated


		* Duplicate mapping keys lead to a`ParseException`.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

javiereguiluz commentedJan 20, 2017

👍 after the changes proposed by@xabbuh

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels

Jan 20, 2017

Taluu reviewed

Jan 20, 2017

View reviewed changes

src/Symfony/Component/Yaml/Parser.php

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

Copy link

Contributor

TaluuJan 20, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

instead of having to do aif func > 0,if func_args > 1, ..., you can leave the optionnal parameters in the constructor (just undocument those) and just check iffunc_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)

Copy link

ContributorAuthor

GuilhemNJan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

[Yaml] Remove internal arguments from the api

e176c09

GuilhemN force-pushed theREMOVEINTERNALARGUMENTS branch from847fda8 toe176c09Compare

January 20, 2017 18:18

Copy link

ContributorAuthor

GuilhemN commentedJan 20, 2017

@xabbuh I fixed your comments.

xabbuh approved these changes

Jan 21, 2017

View reviewed changes

xabbuh requested changes

Jan 21, 2017

View reviewed changes

src/Symfony/Component/Yaml/Parser.php Outdated

		* @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
		*/
		publicfunction__construct($offset =0,$totalNumberOfLines =null,array$skippedLineNumbers =array())