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

Use a modern directory structure #1767

Merged
merged 1 commit into from Mar 15, 2024

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Mar 14, 2024

The recommended directory structure is more similar to what we do in non-bundle repositories.
Also, this means smaller configuration for tools such as PHPCSStandards, PHPUnit or Psalm.

Although this is no bugfix, I'm targeting 2.11.x so that merging up does not become a nightmare.

@greg0ire
Copy link
Member Author

The Psalm failures demonstrate another reason why this is better: the Middleware directory was forgotten in this list:

<directory name="CacheWarmer"/>
<directory name="Command"/>
<directory name="Controller"/>
<directory name="DataCollector"/>
<directory name="Dbal"/>
<directory name="DependencyInjection"/>
<directory name="EventSubscriber"/>
<directory name="Mapping"/>
<directory name="Repository"/>
<directory name="Tests"/>
<directory name="Twig"/>

@ostrolucky
Copy link
Member

yes, this is welcome

@greg0ire
Copy link
Member Author

I'm unsure where the 2 svg files should go. Should they go into public, or should I keep them under templates/Collector? They are referenced in

{% if profiler_markup_version >= 3 %}
{{ include('@Doctrine/Collector/database.svg') }}
{% else %}
<span class="icon">{{ include('@Doctrine/Collector/icon.svg') }}</span>
{% endif %}

@ostrolucky
Copy link
Member

Keep them in templates. And let's target next minor.

@greg0ire greg0ire changed the base branch from 2.11.x to 2.12.x March 15, 2024 06:46
@greg0ire
Copy link
Member Author

Will address the conflicts once #1768 is merged and merged up.

The recommended directory structure is more similar to what we do in
non-bundle repositories.
Also, this means smaller configuration for tools such as PHPCSStandards,
PHPUnit or Psalm.
@greg0ire greg0ire marked this pull request as ready for review March 15, 2024 12:56
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

To support the modern structure, the DoctrineBundle class needs to be updated to override the getPath() method (or to extend AsbtractBundle which already does the override) as the bundle class is not at the root path of the bundle anymore.

}
},
"autoload-dev": {
"psr-4": {
"": "Tests/DependencyInjection"
"": "tests/DependencyInjection",
Copy link
Member

Choose a reason for hiding this comment

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

As this seems to be about autoloading the Fixtures namespace, I would suggest using Fixtures\\ as the PSR-4 prefix (and going one level deeper for the path) instead of a fallback for any class.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will do in a separate PR

@ostrolucky ostrolucky added this to the 2.12.0 milestone Mar 15, 2024
@ostrolucky ostrolucky merged commit fa3e8e3 into doctrine:2.12.x Mar 15, 2024
15 checks passed
@greg0ire greg0ire deleted the rework-dir-structure branch March 15, 2024 14:53
@greg0ire
Copy link
Member Author

To support the modern structure, the DoctrineBundle class needs to be updated to override the getPath() method (or to extend AsbtractBundle which already does the override) as the bundle class is not at the root path of the bundle anymore.

@ostrolucky why did you merge? Is that not an issue?

greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
@greg0ire
Copy link
Member Author

Addressed in #1770

greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Mar 15, 2024
This is required when the bundle class is not at the root path of the
bundle, which is the case since doctrine#1767
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants