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

phpzlc v2 #1444

Closed
wants to merge 18 commits into from
Closed

phpzlc v2 #1444

wants to merge 18 commits into from

Conversation

CJayHe
Copy link
Contributor

@CJayHe CJayHe commented Oct 19, 2022

Q A
License MIT
Packagist

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 19, 2022 09:20
@github-actions
Copy link

github-actions bot commented Oct 19, 2022

Thanks for the PR 😍

How to test these changes in your application

  1. Define the SYMFONY_ENDPOINT environment variable:

    # On Unix-like (BSD, Linux and macOS)
    export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes-contrib/flex/pull-1444/index.json
    # On Windows
    SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes-contrib/flex/pull-1444/index.json
  2. Install the package(s) related to this recipe:

    composer req 'symfony/flex:^1.16'
    composer req 'phpzlc/admin-business:^2.0' 'phpzlc/area-business:^2.0' 'phpzlc/phpzlc:^2.4' 'phpzlc/upload-business:^2.0'
  3. Don't forget to unset the SYMFONY_ENDPOINT environment variable when done:

    # On Unix-like (BSD, Linux and macOS)
    unset SYMFONY_ENDPOINT
    # On Windows
    SET SYMFONY_ENDPOINT=

Diff between recipe versions

In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes.
I'm going keep this comment up to date with any updates of the attached patch.

phpzlc/admin-business

1.0 vs 2.0

phpzlc/phpzlc

1.0 vs 2.0
diff --git a/phpzlc/phpzlc/1.0/manifest.json b/phpzlc/phpzlc/2.0/manifest.json
index 6063d6a2..d83b1037 100644
--- a/phpzlc/phpzlc/1.0/manifest.json
+++ b/phpzlc/phpzlc/2.0/manifest.json
@@ -4,6 +4,7 @@
     },
     "copy-from-package": {
         "Doctrine/ORM/Rewrite/Templates/Repository.tpl.php": "vendor/symfony/maker-bundle/src/Resources/skeleton/doctrine/Repository.tpl.php",
+        "Doctrine/ORM/Rewrite/Hydration/AbstractHydrator.php": "vendor/doctrine/orm/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php",
         "Doctrine/ORM/Rewrite/Hydration/ObjectHydrator.php": "vendor/doctrine/orm/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php",
         "Doctrine/ORM/Rewrite/MakeEntityRegenerate/ClassSourceManipulator.php": "vendor/symfony/maker-bundle/src/Util/ClassSourceManipulator.php",
         "Doctrine/ORM/Rewrite/MakeEntityRegenerate/EntityRegenerator.php": "vendor/symfony/maker-bundle/src/Doctrine/EntityRegenerator.php",
2.0 vs 2.3
2.3 vs 2.4

phpzlc/upload-business

1.0 vs 2.0

auto-merge was automatically disabled October 19, 2022 09:24

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 19, 2022 09:25
auto-merge was automatically disabled October 19, 2022 09:40

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 19, 2022 09:40
auto-merge was automatically disabled October 19, 2022 09:52

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 19, 2022 09:53
auto-merge was automatically disabled October 20, 2022 05:56

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 05:56
auto-merge was automatically disabled October 20, 2022 06:09

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 06:09
auto-merge was automatically disabled October 20, 2022 06:12

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 06:13
auto-merge was automatically disabled October 20, 2022 09:56

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 09:57
auto-merge was automatically disabled October 20, 2022 09:58

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 09:59
auto-merge was automatically disabled October 20, 2022 10:05

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 10:05
auto-merge was automatically disabled October 20, 2022 10:36

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 10:37
auto-merge was automatically disabled October 20, 2022 10:40

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 10:41
auto-merge was automatically disabled October 20, 2022 10:41

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 20, 2022 10:41
auto-merge was automatically disabled October 26, 2022 07:05

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 26, 2022 07:05
auto-merge was automatically disabled October 26, 2022 07:09

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) October 26, 2022 07:10
@CJayHe CJayHe disabled auto-merge December 30, 2022 03:23
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I have a hard figuring out what you're trying to do here.
A recipe is mainly helpful when we want to add some configuration files.
We tend to avoid adding PHP classes as this is more for maker bundle.

"Repository/": "src/Repository/",
"templates/": "templates/",
"DataFixtures/": "src/DataFixtures/"
}
Copy link
Member

Choose a reason for hiding this comment

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

None of these directories exist, so the whole file can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of copying here is to copy some written business code to the corresponding location of the project. These codes can be easily reused and adjusted according to the current project.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I read too fast, that's a copy from package, not recipe.
I think that's something that will work long-term.
Just one example. Your entities are probably using annotations or attributes. Depending on what the user has configured, it will work or not.
So, instead, you should probably have a command that copies/generates/customizes those files. This command to be run can be advertized in a post install message (that's a recommendation I've made in the last few weeks in many similar PRs here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know what you mean, and I've thought about it too, but I think it will make the installation more complicated and inconvenient.

  1. Almost all of my packages need to do this, which means I have to execute extra commands every time.
  2. Executing copy is the same thing as symfony-flex, which means I have developed symfony-flex repeatedly.
  3. symfony-flex is great in my opinion. It was also intended to be easy to install.
  4. These configurations of mine have to work, there is no alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Then, I fear that your packages are not a good fit for recipes :(
We want recipes that work for all use cases, not just one specific one.
To generate files, we have maker bundle, that's probably what you want to extend here.
Recipes are a way to configure things.
Another issue with copying PHP files that need to be heavily changed by the user: when one wants to update their recipes, they will get plenty of conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your guidance.
But for now in my scenario, using symfy-flex is the best solution.

  1. If you change the method, you need to write a component like symfy-flex. The functionality written is not as good as the Symfony-Flex experience, which as a symfony user I don't think is cool.
  2. Secondly, I think even if the method is changed, the problem will not be reduced, because the principle and purpose of the two are the same, so the situation is similar.

"Repository/": "src/Repository/",
"DataFixtures/": "src/DataFixtures/"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

"PHPZlc\\PHPZlc\\Bundle\\PHPZlcBundle": ["all"]
},
"copy-from-package": {
"Doctrine/ORM/Rewrite/Templates/Repository.tpl.php": "vendor/symfony/maker-bundle/src/Resources/skeleton/doctrine/Repository.tpl.php",
Copy link
Member

Choose a reason for hiding this comment

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

What if Marker bundle is not installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

composer package phpzlc/phpzlc will decide who to rely on

@CJayHe CJayHe requested review from fabpot January 3, 2023 04:12
"Repository/": "src/Repository/",
"templates/": "templates/",
"DataFixtures/": "src/DataFixtures/"
}
Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I read too fast, that's a copy from package, not recipe.
I think that's something that will work long-term.
Just one example. Your entities are probably using annotations or attributes. Depending on what the user has configured, it will work or not.
So, instead, you should probably have a command that copies/generates/customizes those files. This command to be run can be advertized in a post install message (that's a recommendation I've made in the last few weeks in many similar PRs here).

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) January 30, 2023 06:04
@CJayHe CJayHe closed this Jan 30, 2023
auto-merge was automatically disabled January 30, 2023 06:08

Pull request was closed

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

2 participants