DanielLeens commented on PR #12450: URL: https://github.com/apache/seatunnel/pull/12450#issuecomment-6097952300
Thanks for the detailed explanation, @goutamadwant, and for testing the Python route against a real WordPress 6.8.3 / WooCommerce 9.9.5 stack before answering. This is a useful data point. I only have read-only review rights here, so the decision on @davidzollo's request is his, but here is the technical context I can add after re-reading the fixture at the current head (`12005b8dd5e`). - The PHP files are not an extra runtime dependency of the connector. `seed.php` and `setup.php` are copied by `WooCommerceIT` into the `wordpress:6.8.3-php8.3-apache` container (`WooCommerceIT.java:89-94`) and executed there with `php /tmp/seed.php` (`WooCommerceIT.java:122,170,176,250`). PHP is already the container's own runtime, so nothing new is required on the CI runner or in the SeaTunnel distribution. - The fixture needs PHP for the reason you gave: it creates orders through `wc_create_order()` and `set_date_created(...)` (`seed.php:32-33`) so that the same historical dates are written correctly in both the posts-based and HPOS storage layouts. As you found, the REST API ignores a supplied `date_created_gmt` and WP-CLI rejects `--date_created`, so a Python-only version would have to write straight into the WooCommerce tables. That would be less faithful to the real order model and more fragile across WooCommerce versions, which would weaken the UTC-boundary tests this E2E exists for. - If the underlying concern is language preference or maintainability, a middle ground is what you already suggested: Python (or the existing Java test code) orchestrates the flow and issues a single narrow `wp eval-file` call. Note that this still executes PHP inside the model, so it mostly changes who owns the glue code rather than removing PHP. The repo already has non-Java E2E helpers (for example `connector-python-e2e/.../emit_rows.py` and the Kafka/Pulsar shell scripts), so a small helper script is not unusual here. My suggestion is to keep the PHP fixture as is, with a short comment at the top of `seed.php` explaining why the order model is used instead of REST/CLI/direct SQL. That keeps the E2E faithful and would give reviewers the rationale in the code itself. Please let @davidzollo confirm whether that is acceptable; I have no new source-level findings on this head and nothing else from my earlier approval has changed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
