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

[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock #29569

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
4 participants
@SerkanYildiz
Copy link
Contributor

SerkanYildiz commented Dec 11, 2018

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets /
License MIT

When phpDocumentor isn't installed the debug:autowiring returns an empty string for class description. With this fallback the end user will get a description even phpDocumentor isn't installed.

Edit: Usage of phpDocumentor is completely removed from debug:autowiring command.

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from c8dafb6 to 562bbda Dec 11, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 11, 2018

Thanks for submitting! During the SymfonyCon hackday last Saturday, someone worked exactly on this - except that they opened no PR so the code might be lost :)

Here are my suggestions:

  • consider this requirement for phpDocumentor as a bug - it's really broken to rely on it, DX wise
  • thus, rebase/retarget this for 4.2
  • let's completely remove the code path that uses phpDocumentor and parse inline ourselves - that's simple enough.
@SerkanYildiz

This comment has been minimized.

Copy link
Contributor

SerkanYildiz commented Dec 11, 2018

Hi @nicolas-grekas,

It was me during the SymfonyCon who started with this patch. Will apply your comments and update this PR. Thanks for your review!

@SerkanYildiz SerkanYildiz changed the base branch from master to 4.2 Dec 11, 2018

@SerkanYildiz SerkanYildiz changed the base branch from 4.2 to master Dec 11, 2018

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from 7cc7cb7 to 1670c27 Dec 11, 2018

@SerkanYildiz SerkanYildiz requested review from dunglas , lyrixx , sroze and xabbuh as code owners Dec 11, 2018

@SerkanYildiz SerkanYildiz changed the base branch from master to 4.2 Dec 11, 2018

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from 1670c27 to 41b5a0c Dec 11, 2018

$summary = str_replace(
array('/', '*', "\t", "\r\n", "\n", "\r", ' '),
'',
substr($docComment, 0, strpos($docComment, '@'))

This comment has been minimized.

@ro0NL

ro0NL Dec 11, 2018

Contributor

this seems very fragile (perhaps * @ works better). But tend to prefer an explicit regex to capture the short description as a sentence.

This captures both short+long description and removes newlines in between.

This comment has been minimized.

@SerkanYildiz

SerkanYildiz Dec 12, 2018

Contributor

@ro0NL Can you help me with a regex to get the description? Would be great!

This comment has been minimized.

@ro0NL

ro0NL Dec 12, 2018

Contributor

@SerkanYildiz something like https://regex101.com/r/Sqcmmf/1/

It matches the first sentence per https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#51-summary

A Summary MUST end with two sequential line breaks, unless it is the only content in the PHPDoc.

The current regex does not include single new lines, e.g. short line 1\nshort line 2 would only match short line 1. AFAIK it's OK.

->getSummary();
}
} catch (\ReflectionException | \InvalidArgumentException $e) {
$docComment = $r->getDocComment();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 11, 2018

Member

if ($docComment = $r->getDocComment()) { should be kept imho

This comment has been minimized.

@SerkanYildiz

SerkanYildiz Dec 12, 2018

Contributor

re-added the if statement thanks @nicolas-grekas

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from 41b5a0c to ca79832 Dec 12, 2018

@SerkanYildiz

This comment has been minimized.

Copy link
Contributor

SerkanYildiz commented Dec 12, 2018

@ro0NL Your regex worked and made the tests pass (at least locally.) Thank You!

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch 2 times, most recently from a6238e1 to 6cbcc61 Dec 12, 2018

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch 2 times, most recently from bc47acd to e25ef7b Dec 12, 2018

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Dec 12, 2018

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

could you please add tests with several variants of class descriptions (or missing ones)?

@SerkanYildiz

This comment has been minimized.

Copy link
Contributor

SerkanYildiz commented Dec 13, 2018

@nicolas-grekas I don't now if it's necessary to add more tests. There were existing tests for the old implementation. We just changed the logic and IMO we just had to ensure that the tests should still pass

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 13, 2018

we should add a test for

/**
 * line1
 * line2
 *
 * line 4
 */

resulting in line1 line2

and

/**
 *oops
 *
 * @annot
 */

resulting in oops

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch 2 times, most recently from 5367eb8 to 7c7e9d6 Dec 13, 2018

@SerkanYildiz

This comment has been minimized.

Copy link
Contributor

SerkanYildiz commented Dec 13, 2018

@nicolas-grekas failing test is not related to this PR

@SerkanYildiz SerkanYildiz force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from a39f599 to b068bb7 Dec 14, 2018

@ro0NL
Copy link
Contributor

ro0NL left a comment

i think we're there :) last minor comments from me.

@ro0NL

ro0NL approved these changes Dec 14, 2018

@SerkanYildiz

This comment has been minimized.

Copy link
Contributor

SerkanYildiz commented Dec 17, 2018

ping @nicolas-grekas , failing AppVeyor seems unrelated. Can we retrigger AppVeyor build?

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] Implement fallback for class descriptor in debug:autowiring [FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock Dec 17, 2018

@nicolas-grekas nicolas-grekas force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from 68b298d to efff8eb Dec 17, 2018

@nicolas-grekas nicolas-grekas force-pushed the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch from efff8eb to 485ed4d Dec 17, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 17, 2018

Thank you @SerkanYildiz.

@nicolas-grekas nicolas-grekas merged commit 485ed4d into symfony:4.2 Dec 17, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 17, 2018

bug #29569 [FrameworkBundle] decouple debug:autowiring from phpdocume…
…ntor/reflection-docblock (SerkanYildiz)

This PR was merged into the 4.2 branch.

Discussion
----------

[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | /
| License       | MIT

When phpDocumentor isn't installed the *debug:autowiring* returns an empty string for class description. With this fallback the end user will get a description even phpDocumentor isn't installed.

**Edit:** Usage of phpDocumentor is completely removed from debug:autowiring command.

Commits
-------

485ed4d [FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock

@SerkanYildiz SerkanYildiz deleted the SerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions branch Dec 17, 2018

@fabpot fabpot referenced this pull request Jan 6, 2019

Merged

Release v4.2.2 #29803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment