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

Remove @​deprecated stuff #3518

Merged
merged 11 commits into from
May 23, 2019
Merged

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Apr 15, 2019

Q A
Type improvement
BC Break yes

Summary

Removes so far @​deprecated stuff for DBAL 3.0. Fixes #3508.

@Majkl578
Copy link
Contributor Author

Majkl578 commented Apr 15, 2019

@morozov There will be rebase conflicts due to changes in Type so we can wait until develop is up-to-date and I'll rebase the PR afterwards.

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Looks good so far. A few notes:

  1. Configuration::setFilterSchemaAssetsExpression() is also deprecated and should be removed.
  2. There are a few cases with trigger_error(/* ... */, E_DEPRECATED).

public const NAMED_TOKEN = '(?<!:):[a-zA-Z_][a-zA-Z0-9_]*';
/**#@-*/
private const POSITIONAL_TOKEN = '\?';
private const NAMED_TOKEN = '(?<!:):[a-zA-Z_][a-zA-Z0-9_]*';

// Quote characters within string literals can be preceded by a backslash.
public const ESCAPED_SINGLE_QUOTED_TEXT = "(?:'(?:\\\\\\\\)+'|'(?:[^'\\\\]|\\\\'?|'')*')";
public const ESCAPED_DOUBLE_QUOTED_TEXT = '(?:"(?:\\\\\\\\)+"|"(?:[^"\\\\]|\\\\"?)*")';
public const ESCAPED_BACKTICK_QUOTED_TEXT = '(?:`(?:\\\\\\\\)+`|`(?:[^`\\\\]|\\\\`?)*`)';
private const ESCAPED_BRACKET_QUOTED_TEXT = '(?<!\b(?i:ARRAY))\[(?:[^\]])*\]';
Copy link
Member

Choose a reason for hiding this comment

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

These constants should also be deprecated and made private.

@morozov
Copy link
Member

morozov commented Apr 16, 2019

Also, SQLSrvStatement::LAST_INSERT_ID_SQL could be deprecated and made private.

@Majkl578
Copy link
Contributor Author

Rebased.

/**
* @dataProvider dataGetPlaceholderPositions
*/
public function testGetPlaceholderPositions($query, $isPositional, $expectedParamPos)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be reworked in two tests, not deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be testing a deleted method.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the method was split apart into two other methods, not just deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm but the test wasn't? That's weird. Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the methods are private.
How do you suggest to test them? Reflection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split into two new tests and two data providers, using reflection - if you have better idea let me know. :)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but the test wasn't? That's weird. Ok.

I didn't notice it at that time.

[…] if you have better idea let me know. :)

It's good enough to me.

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 8, 2019

Why was this PR closed? @morozov @jwage?

@jwage
Copy link
Member

jwage commented May 8, 2019

Work is being continued here #3538

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 8, 2019

That is not a valid reason to close an open PR that was not inactive - no one even asked me to rebase this PR or anything else. You are basically only stealing my work.

@jwage
Copy link
Member

jwage commented May 8, 2019

@Majkl578 Happy to continue the work in here. Since you closed many other PRs and deleted the branches I assumed you were not interested in continuing the work. Apologies if that assumption was incorrect. Would you like to continue the work here?

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 8, 2019

I closed PRs in other repos because I lost interest in contributing to an organization that treats members like gabrage.
Regarding PRs in DBAL @morozov and me came up with a list of issues to either finish or abandon, those open are active and to be finished either by me, by @morozov or both.

@jwage
Copy link
Member

jwage commented May 8, 2019

Understood. @morozov asked if I could finish #3518 and I obliged. Would you like to finish the pull request here or make the branch open so that we can continue the work?

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 8, 2019

I will do it here but at this moment there are no requested changes here - the PR was simply ready for review/merge (except maybe rebase), not aware of anything needed to finish #3518.

@jwage
Copy link
Member

jwage commented May 8, 2019

Cool. Can you rebase? Then @morozov and I will help get it reviewed and merged.

@jwage jwage reopened this May 8, 2019
@Majkl578
Copy link
Contributor Author

Majkl578 commented May 8, 2019

Sure, will do soon.

@morozov morozov force-pushed the drop-deprecated branch from 673c62c to 1993fc2 Compare May 23, 2019 18:38
@morozov morozov removed the WIP label May 23, 2019
@morozov morozov self-assigned this May 23, 2019
@morozov morozov merged commit d1c6699 into doctrine:develop May 23, 2019
@morozov
Copy link
Member

morozov commented May 23, 2019

Thanks @Majkl578!

@Majkl578 Majkl578 deleted the drop-deprecated branch May 23, 2019 19:04
morozov added a commit to morozov/dbal that referenced this pull request May 31, 2019
morozov added a commit to morozov/dbal that referenced this pull request May 31, 2019
morozov added a commit to morozov/dbal that referenced this pull request May 31, 2019
morozov added a commit that referenced this pull request Jun 13, 2019
morozov added a commit that referenced this pull request Jun 27, 2019
morozov added a commit that referenced this pull request Jun 27, 2019
morozov added a commit that referenced this pull request Jun 27, 2019
morozov added a commit to morozov/dbal that referenced this pull request Aug 26, 2019
morozov added a commit to morozov/dbal that referenced this pull request Aug 27, 2019
morozov added a commit that referenced this pull request Nov 2, 2019
@morozov morozov modified the milestones: 4.0.0, 3.0.0 Jun 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants