8000 [Config] Add `NodeDefinition::docUrl()` by alexandre-daubois · Pull Request #59762 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Add NodeDefinition::docUrl() #59762

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 4, 2025
Merged

[Config] Add NodeDefinition::docUrl() #59762

merged 1 commit into from
Apr 4, 2025

Conversation

alexandre-daubois
Copy link
Member
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

Adding such information would allow extensions and bundles to provide even more info with a documentation "one click away".

The primary goal is to use this feature in conjunction with #58771, allowing to dump a @see https://symfony.com/doc/... right next to the configuration array shape.

@carsonbot carsonbot added this to the 7.3 milestone Feb 12, 2025
@GromNaN
Copy link
Member
GromNaN commented Feb 12, 2025

This can also be used in the Yaml JsonSchema: #59620

@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch 2 times, most recently from acbda3f to b9240b9 Compare February 13, 2025 08:16
@alexandre-daubois
Copy link
Member Author

Should we bump the minimum requirement for symfony/config to ^7.3 in bundles, or conditionally call documentationUri() on the root node?

@stof
Copy link
Member
stof commented Feb 13, 2025

I suggest bumping the min version instead of doing conditional calls. It makes the code simpler

@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch 4 times, most recently from 6d8ef52 to b2502a8 Compare February 13, 2025 17:53
@stof
Copy link
Member
stof commented Feb 13, 2025

You should probably also update the dumpers to add a comment with the documentation URL (when present), so that debug:config and config:dump-reference show it.

@alexandre-daubois
Copy link
Member Author

Right, here's the result with the latest push:

Capture d’écran 2025-02-14 à 10 14 52 Capture d’écran 2025-02-14 à 10 07 21

@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch 2 times, most recently from 3a00a22 to c7f73c8 Compare February 19, 2025 12:43
@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch from c7f73c8 to d6884cc Compare March 19, 2025 10:58
@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch from d6884cc to 168ede3 Compare March 25, 2025 11:01
@alexandre-daubois alexandre-daubois changed the title [Config] Add NodeDefinition::documentationUri() [Config] Add NodeDefinition::docUrl() Mar 25, 2025
alexandre-daubois
alexandre-daubois commented Mar 25, 2025
View reviewed changes
src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php
public function docUrl(string $uri, ?string $package = null): static
{
if ($package) {
preg_match('/^(\d+)\.(\d+)\.(\d+)/', InstalledVersions::getVersion($package) ?? '', $m);
Copy link
Member Author

Choose a reason for hiding this comment

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

No $, because some packages may return a fourth number (in local, phpunit/phpunit returns 9.6.99.0 for example on my computer)

@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch from 168ede3 to 1d3c7b3 Compare March 28, 2025 13:46
@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch from 1d3c7b3 to a27e4aa Compare March 28, 2025 13:46
@alexandre-daubois alexandre-daubois force-pushed the config-documentation-uri branch from a27e4aa to 3c7fce2 Compare April 3, 2025 11:53
fabpot
fabpot reviewed Apr 4, 2025
View reviewed changes
src/Symfony/Bundle/DebugBundle/composer.json
@@ -18,13 +18,14 @@
"require": {
"php": ">=8.2",
"ext-xml": "*",
"composer-runtime-api": ">=2.1",
Copy link
Member

Choose a reason for hiding this comment

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

When installing the smallest possible Symfony app, we currently only require Symfony deps. So, that would be the first non-Symfony dependency that we require.

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought until I realized the framework bundle already has this dependency:
https://github.com/symfony/symfony/blob/7.3/src/Symfony/Bundle/FrameworkBundle/composer.json

@fabpot
Copy link
Member
fabpot commented Apr 4, 2025

Thank you @alexandre-daubois.

@fabpot fabpot merged commit 2795d95 into symfony:7.3 Apr 4, 2025
9 of 11 checks passed
@fabpot fabpot mentioned this pull request Apr 4, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
@xabbuh xabbuh mentioned this pull request May 4, 2025
Merged
fabpot added a commit that referenced this pull request May 4, 2025
@fabpot
minor #60341 [DebugBundle] require the 7.3+ of the Config component (…
2b9f361 
…xabbuh)

This PR was merged into the 7.3 branch.

Discussion
----------

[DebugBundle] require the 7.3+ of the Config component

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

related to #59762, we could bump the conflict rule instead, but I don't see a use case where using the bundle without the Config component actually makes sense

Commits
-------

28d1a83 require the 7.3+ of the Config component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@GromNaN GromNaN GromNaN left review comments

@stof stof stof left review comments

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Config Feature Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
7 participants
@alexandre-daubois @GromNaN @stof @fabpot @nicolas-grekas @OskarStark @carsonbot
0