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

changes for using latest shopware flex template #3586

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

xavgru12
Copy link
Contributor

  • Bug fix: the theme is missing on deployed version. Compiling the theme in the sw:deploy task fixes it. Another fix: in CI systems there is no way to compile the theme locally since a database connection is needed. Adding the theme compile skip option at the build-js.sh fixes it as well.

@antonmedv antonmedv merged commit 87f196e into deployphp:master Apr 29, 2023
@UlrichThomasGabor
Copy link
Contributor

What exactly was not working? I don't understand from this issue here.

The recipe was implemented having this site in mind: build SW without DB, or alternatively this one official PaaS documentation. Building should happen locally and it was working without a database connection before, because the mentioned files were obtained from the server. From what I understand, you have disabled part of the building? When will building the theme happen now?

From what I see at the moment, I think merging this PR broke things, and without further explanation I think it should be reverted.

@antonmedv @Schrank

@Schrank
Copy link
Contributor

Schrank commented Oct 19, 2023

Unfortunately I'm not using the shopware recipe at the moment, but a very old version which works for us.

@xavgru12 did you follow any of the mentioned links by @UlrichThomasGabor? Because to build it locally you need to adjust your configuration.

I tend to revert it but add a paragraph to the config about the two links Ulrich provided, so people see in the recipe how to use it. Any comments on this idea?

@UlrichThomasGabor
Copy link
Contributor

Sounds good to me.

@UlrichThomasGabor
Copy link
Contributor

UlrichThomasGabor commented Oct 19, 2023

When will building the theme happen now?

I just saw that you added a remote task for that. While I guess this is working, it is in my opinion unnecessary to compile the theme on each deployed instance, when all data is already available to compile locally (saves time and energy).

Without further explanation on what problem this PR is actually solving, I would still vote for a revert.

@xavgru12
Copy link
Contributor Author

I have followed the guide of compiling without Database connection.
Using without database connection, I had to skip the theme compilation as it was described in the guide.

This resulted in the deployed version not having the theme compiled. Adding the remote task resolves this.

@UlrichThomasGabor
Copy link
Contributor

You are referring to the last part?
https://developer.shopware.com/docs/guides/hosting/installation-updates/deployments/build-w-o-db.html#partially-compiling-the-storefront

This is optional. You can add that via a switch, but I don't think it is good practice to change the behavior for everyone.

@xavgru12
Copy link
Contributor Author

Compiling the storefront without database does not work for me following the guide. Using the theme skip works fine, however.

@UlrichThomasGabor
Copy link
Contributor

Feel free to open an issue or ask on Stackoverflow or SW Slack for help, if you encounter problems with the setup. You can also change your recipe locally (redefine the tasks). But in my opinion, the standard case should be the default one. It is definitely working, since we are using this process for 2 years now or something like that.

UlrichThomasGabor added a commit to UlrichThomasGabor/deployer that referenced this pull request Oct 24, 2023
UlrichThomasGabor added a commit to UlrichThomasGabor/deployer that referenced this pull request Oct 24, 2023
UlrichThomasGabor added a commit to UlrichThomasGabor/deployer that referenced this pull request Oct 24, 2023
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

4 participants