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

Added documentation for bulk entity extension #1543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

OliverSkroblin
Copy link
Contributor

@OliverSkroblin OliverSkroblin commented Nov 7, 2024

public function collect(): \Generator
{
yield 'product' => [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield 'product' => [
yield \Shopware\Core\Content\Product\ProductDefinition::ENTITY_NAME => [

should we encourage the usage of the constants? 🤔

Copy link
Contributor Author

@OliverSkroblin OliverSkroblin Nov 7, 2024

Choose a reason for hiding this comment

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

I see what you mean. I would document it in this way because Entities via Attributes or custom entities don't have such constants and the constant is also no "official" requirement right?
It also refer to the "definition" again, something which might be obsolete in the future

Therefore i would recommend to show it in this way i guess.

Copy link
Contributor

@JoshuaBehrens JoshuaBehrens Nov 7, 2024

Choose a reason for hiding this comment

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

@OliverSkroblin yes and the entities via attributes are a hell without these constants! I will provide a fix soon, that allows using the AttributeEntity::class as reference. This is such a mess as this will hold us back from using it the Doctrine ORM way.

It was really difficult to find out how to extend an attribute entity >:(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that such a hassle? You can simply reference the table name. You don’t always have to use constants everywhere ;).

If this is important for all of you, i would recommend to add a class constant again. I would always go the way of using a simple string which make it much more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fun thing. I would always go for the PHP symbol so I can ensure the "magic string" is stored on a symbol, that is affected by refactorings correctly. This helps to understand by just looking at the uses what my real-life dependencies are. I see the benefit in the simplicity at time of writing, not at time of maintaining.

@Isengo1989 Isengo1989 added the Blocked Block PRs from merging label Nov 22, 2024
@AydinHassan AydinHassan self-assigned this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Block PRs from merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants