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

feat: add contao project type #5672

Closed
wants to merge 1 commit into from
Closed

Conversation

Morgy93
Copy link
Sponsor Collaborator

@Morgy93 Morgy93 commented Jan 2, 2024

The Issue

https://contao.org/en/ is not supported officially.

How This PR Solves The Issue

It adds a new project type called contao that adds native support.

Manual Testing Instructions

Follow new CMS Quickstart section for contao.

https://github.com/ddev/ddev/pull/5672/files#diff-cdacb5ab99042792ddb67ee7d62ac1d36c4699992949005880517b4955e10f7e

Automated Testing Overview

It extends the TestDdevFullSiteSetup to test Contao.

https://github.com/Morgy93/ddev-test-contao is used for the test environment which should probably be moved to the DDEV namespace.

Release/Deployment Notes

Add a new project typed called contao for using the CMS https://contao.org/en/.

Copy link

github-actions bot commented Jan 2, 2024

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Wow, look at you go!

@Morgy93 Morgy93 force-pushed the feature/add-contao branch 4 times, most recently from 508ea8d to 01f4b41 Compare January 3, 2024 21:04
@Morgy93 Morgy93 force-pushed the feature/add-contao branch 3 times, most recently from e6bcc39 to 1f533f9 Compare January 18, 2024 12:11
@Morgy93 Morgy93 marked this pull request as ready for review January 18, 2024 12:52
@Morgy93 Morgy93 requested review from a team as code owners January 18, 2024 12:52
@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Jan 18, 2024

The errors in the checks don't seem to be related to changes in this PR:


TYPO3:

--- FAIL: TestDdevFullSiteSetup (1091.01s)
FAIL
2024-01-18T13:06:53.245 TYPO3 version class not found in '/home/runner/tmp/ddevtest/TestPkgTypo31717143745/vendor/typo3/cms-core/Classes/Information/Typo3Version.php' for project TestPkgTypo3, installed version is assumed to be older than 11.5.0: open /home/runner/tmp/ddevtest/TestPkgTypo31717143745/vendor/typo3/cms-core/Classes/Information/Typo3Version.php: no such file or directory
FAIL	github.com/ddev/ddev/pkg/ddevapp	3117.023s

https://github.com/ddev/ddev/actions/runs/7569757491/job/20613740279?pr=5672


xdebug:

--- FAIL: TestDdevXdebugEnabled (28.87s)
FAIL
FAIL	github.com/ddev/ddev/pkg/ddevapp	1010.923s
FAIL
make: *** [Makefile:126: testddevapp] Error 1

https://github.com/ddev/ddev/actions/runs/7569757491/job/20613739351?pr=5672


Mutagen:

FAIL: TestMutagenSimple (3391.44s) | 1s
-- | --
  | FAIL
  | FAIL	github.com/ddev/ddev/pkg/ddevapp	8080.194s
  | FAIL
  | make: *** [Makefile:126: testddevapp] Error 1

https://buildkite.com/ddev/wsl2-docker-inside/builds/3105#018d1c7b-d9b5-4916-924a-9bf5cbb89835


I did not touch any of that. 🤔

@rfay Any idea?

@stasadev
Copy link
Member

stasadev commented Jan 18, 2024

TYPO3:

Updating /home/runner/tmp/ddevtest/TestPkgContao914301743/.env.local with map[DATABASE_URL:***db/db MAILER_DSN:smtp://127.0.0.1:1025]
...
http status code for 'http://127.0.0.1:33194' was 301, not 200

https://github.com/ddev/ddev/actions/runs/7569757491/job/20613740279?pr=5672#step:17:7053

I see TestPkgContao here, looks like it's related.


xdebug:

String does not contain "xdebug.client_host => host.docker.internal => host.docker.internal"

https://github.com/ddev/ddev/actions/runs/7569757491/job/20613739351?pr=5672#step:17:3300

Seems like an unrelated issue to me. Restarted.


Mutagen:

composer: A connection timeout was encountered.

https://buildkite.com/ddev/wsl2-docker-inside/builds/3105#018d1c7b-d9b5-4916-924a-9bf5cbb89835/9635-9889

Saw this in recent PRs, some temporary issue, unrelated. Restarted.

@rfay
Copy link
Member

rfay commented Jan 19, 2024

Looks like you're making great progress!

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Jan 19, 2024

Thanks for the review, it was helpful.

http status code for 'http://127.0.0.1:33194' was 301, not 200

https://github.com/ddev/ddev/actions/runs/7569757491/job/20613740279?pr=5672#step:17:7053

I see TestPkgContao here, looks like it's related.

I don't understand why it uses http://127.0.0.1:33194.
Because it redirects via 301 to https://testpkgcontao.ddev.site/ legitimately.

The check works fine using https://testpkgcontao.ddev.site/.

pkg/ddevapp/ddevapp_test.go line 2150: rawurl := app.GetPrimaryURL() + site.DynamicURI.URI
The rawurl returns https://testpkgcontao.ddev.site/ as well.

The following body, resp, err := testcommon.GetLocalHTTPResponse(t, rawurl, 120) body variable contains the valid context as well.

@stasadev Any idea what's wrong here?

@rfay
Copy link
Member

rfay commented Jan 19, 2024

It sounds like the test contao project doesn't work without a specific hostname in the URL? The direct port-access accesses the web container directly, whereas https://testpkgcontao.ddev.site/ goes through the ddev-router.

You can experiment with this locally, use the direct-port URL provided by ddev describe

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Jan 19, 2024

It sounds like the test contao project doesn't work without a specific hostname in the URL? The direct port-access accesses the web container directly, whereas https://testpkgcontao.ddev.site/ goes through the ddev-router.

You can experiment with this locally, use the direct-port URL provided by ddev describe

I think I misinterpreted something in my previous answer:

The test checks the HTTP URL: http://127.0.0.1:33194
But it is redirected via 301 to HTTPS: https://127.0.0.1:33194

So I need to make sure that the page works with IP + HTTP only, right?

@rfay
Copy link
Member

rfay commented Jan 19, 2024

Well, it's your test you're writing, correct? So you'll want to make sure that the test covers things that Contao should be able to do.

Most project types can handle http or https with or without a hostname in the URL. I imagine contao can also. But the big picture idea is to write a test that will make sure Contao always works when it should.

Remember that you can run the test locally, you don't have to wait for the automated test response.

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Jan 19, 2024

Well, it's your test you're writing, correct? So you'll want to make sure that the test covers things that Contao should be able to do.

It's the default TestDdevFullSiteSetup test. I am unaware of how much I can manipulate it "to make it work".
I see occasionally if site.Name stuff, but I'd like to stick to the standards as much as possible.

I run the tests locally (e.g. GOTEST_SHORT=18 make test TESTARGS="-run TestDdevFullSiteSetup"), but the results differ from setting up the project manually.
I'd love to be able to replicate the behavior more closely.

Sidenote: It's much fun to setup a new project type. Coming across so many inner workings of DDEV to understand it better. Just awesome. 😊

@rfay
Copy link
Member

rfay commented Jan 19, 2024

It's the default TestDdevFullSiteSetup test. I am unaware of how much I can manipulate it "to make it work".

Sorry, I wasn't paying careful attention!

So glad you're having fun! Your many contributions are much appreciated!

You should mostly get the same results with your GOTEST_SHORT=18 make test TESTARGS="-run TestDdevFullSiteSetup", not sure why you'd be seeing anything else.

@rfay
Copy link
Member

rfay commented Jan 19, 2024

You can also step through the test in GoLand or vscode. That may help you understand what's happening.

@Morgy93 Morgy93 marked this pull request as draft January 19, 2024 16:46
@rfay
Copy link
Member

rfay commented Feb 2, 2024

Rebased. This is still in draft mode, is it ready for review? How do you feel about it @Morgy93 ?

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Feb 4, 2024

The TestDdevFullSiteSetup is failing and I don't understand why.

GOTEST_SHORT=18 make test TESTARGS="-run TestDdevFullSiteSetup" results in:

http status code for 'https://127.0.0.1/' was 301, not 200

But it works fine running it manually:

  1. Get the DDEV PR binary and use it.
  2. Download https://github.com/Morgy93/ddev-test-contao/archive/refs/tags/5.2.tar.gz and extract
  3. cd to/the/extracted/directory/ddev-test-contao-5.2
  4. ddev config --project-type=contao --docroot=public
  5. ddev start
  6. Download https://github.com/Morgy93/ddev-test-contao/releases/download/5.2/contao_db.sql.tar.gz and import via ddev import-db
  7. ddev ssh
  8. curl -v https://127.0.0.1
  9. See that no 301 redirect is happening.

And I don't know why the test behaves differently.

@rfay
Copy link
Member

rfay commented Feb 4, 2024

This means that the codebase you've installed does a 301 redirect. You should be able to step through the test and stop at that point and see what's been installed and figure out why it's redirecting. I imagine it's just the tarball you're using.

@stasadev
Copy link
Member

Rebased to resolve conflicts.

@stasadev
Copy link
Member

stasadev commented Feb 29, 2024

Tested quickstart manually:

curl -I https://127.0.0.1
HTTP/1.1 301 Moved Permanently
Date: Thu, 29 Feb 2024 20:30:42 GMT
Server: Apache/2.4.57 (Debian)
Referrer-Policy: no-referrer-when-downgrade, strict-origin-when-cross-origin
Permissions-Policy: interest-cohort=()
Contao-Cache: miss
Location: http://127.0.0.1/
Content-Type: text/html; charset=UTF-8

And at the same time https://my-contao-project.ddev.site/contao opens fine under https.
https://my-contao-project.ddev.site redirects to http://my-contao-project.ddev.site.

Need to look for redirect settings https://stackoverflow.com/questions/61632993/avoid-multiple-redirects-in-contao
There is something for Apache, maybe we need to add something to nginx.

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Mar 4, 2024

Tested quickstart manually:

curl -I https://127.0.0.1
HTTP/1.1 301 Moved Permanently
Date: Thu, 29 Feb 2024 20:30:42 GMT
Server: Apache/2.4.57 (Debian)
Referrer-Policy: no-referrer-when-downgrade, strict-origin-when-cross-origin
Permissions-Policy: interest-cohort=()
Contao-Cache: miss
Location: http://127.0.0.1/
Content-Type: text/html; charset=UTF-8

And at the same time https://my-contao-project.ddev.site/contao opens fine under https. https://my-contao-project.ddev.site redirects to http://my-contao-project.ddev.site.

Need to look for redirect settings https://stackoverflow.com/questions/61632993/avoid-multiple-redirects-in-contao There is something for Apache, maybe we need to add something to nginx.

Thanks very much for testing. I saw that the data on the provided repo is somehow different from my local setup. (Where the data was extracted from)

Will have a look what's going on there. Just terribly busy now. 😒

@rfay
Copy link
Member

rfay commented Mar 4, 2024

Thanks for your work on this!

@rfay
Copy link
Member

rfay commented Mar 4, 2024

I've created ddev/test-contao and given you full privileges on it. Like https://github.com/ddev/test-cakephp - the idea is that the testing artifacts should be able to be built from that repo predictably, and have instructions on how to build and maintain them. Thanks!

@stasadev
Copy link
Member

stasadev commented Mar 5, 2024

Rebased to resolve conflicts (without running tests).

Copy link
Collaborator

@rpkoller rpkoller left a comment

Choose a reason for hiding this comment

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

i've left two small nitpicks in regards of the quick start guide section. but when trying the actual quick start guide i ran into an error on the composer create step: https://gist.github.com/rpkoller/d1d79bee1fbfd54b7e0b5c53d8b1e582

@@ -47,6 +47,35 @@ ddev launch
ddev launch
```

## Contao

=== "Composer"
Copy link
Collaborator

@rpkoller rpkoller Mar 21, 2024

Choose a reason for hiding this comment

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

I would remove the tab. contao would be the sole project type that has a single tab. if there is only content for a single tab then place the text as is imho.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Contao offers two install methods:

  • Composer
  • Contao Manager

I wanted to support both, therefore the tab.

Plus, Contao offers a demo which could be helpful, so there could be three tabs:

  • Composer
  • Contao Manager
  • Demo

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahhhh then it is totally fine, i've just noticed the single tab looking at the preview: https://ddev--5672.org.readthedocs.build/en/5672/ but i am not sure if it is necessary to a demo tab which is probably including the link to the demo site? a tab for each of the two install methods (composer and contao manager) is probably enough?

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

I did not know that there is a preview rendered for the docs. Thanks for pointing me to it.

The demo site is something you can install to play around with locally. So, it has different install instructions.


```shell
# Create a project directory and move into it:
mkdir my-contao-project && cd $_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mkdir my-contao-project && cd $_
mkdir my-contao-project
cd my-contao-project

In regards of consistency i would stick to the pattern other quickstart guides use and write out the folder name on the change directory step, even it is longer

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Mar 21, 2024

when trying the actual quick start guide i ran into an error on the composer create step: https://gist.github.com/rpkoller/d1d79bee1fbfd54b7e0b5c53d8b1e582

That's due to Mutagen. I have no clue how to handle it, other than removing the uploadDirs feature.

@rfay
Copy link
Member

rfay commented Mar 25, 2024

Failed to remove directory "/var/www/html/public/files": rmdir(/var/www/html/public/files): Device or resource busy

This is a result of the composer.json script trying to delete the directory specified by upload_dirs. When mutagen is turned on (most macOS and traditional Windows systems), the upload_dirs directories are bind-mounted, which means it's an error to try to remove them. It's silly that the composer.json script tried to delete the directory instead of its contents, but let's try this workaround in the script:

ddev config --upload-dirs=.ddev/tmp
ddev composer create ...
ddev config --upload-dirs=""

I think that will set it up so that everything works OK. It would be better to make the composer script smarter, but we can work with what we have.

@rfay
Copy link
Member

rfay commented Mar 25, 2024

This isn't the only composer.json that has a plugin doing this kind of thing. We should consider solving this in ddev composer create, which is mostly when it happens. Unfortunately, it may require an extra restart. /cc @stasadev

@Morgy93
Copy link
Sponsor Collaborator Author

Morgy93 commented Apr 23, 2024

I am unable to reproduce the error 500 locally, therefore I need to close this PR for now.

Hopefully coming back to it later. Feeling so close, yet so far~

@Morgy93 Morgy93 closed this Apr 23, 2024
@rfay
Copy link
Member

rfay commented Apr 23, 2024

I'll be happy to work with you on this! Thanks for the initiative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants