search

magento / magento2-upward-connector

Magento module for routing front-end requests to UPWARD-PHP
Open Software License 3.0
24 stars 27 forks source link

[TASK] PWA area, Enable / Disable PWA, Dynamically set SERVER variables, Default frontend fallback #12

Closed ghost closed 4 years ago

ghost commented 5 years ago
ghost commented 5 years ago

@tjwiebell I've created a pull request for the features mentioned in issue #11

ghost commented 5 years ago

@bbatsche @tjwiebell Have you had the opportunity to look at this? If the adjustments are not relevant for this repo let me know. Then I will continue on a different version. Currently I also implement the transfer of the state (session) between the PWA and the regular Magento frontend. Let me know! Thanks a lot. Derrick

zetlen commented 5 years ago

Hi @dheesbeen . Thanks so much for volunteering this.

I should start by saying I'm not a PHP developer, so I can't have the final word on code review for this. But I can ask a few high-level questions.

  1. Why would you enable certain controllers, rather than certain route patterns?
  2. Can you describe what uses cases which need the store view selector? We want the connection between a deployed PWA and the Magento store to be as one-way as possible. The Magento instance functioning "headlessly" should have minimal knowledge of the frontend. This is to avoid tight coupling and also to allow for many-to-one scenarios, where multiple PWAs can use the same backing Magento instance with no problems. (The key use case for this is a frontend developer team working on a new project, who can all use a shared internal staging store.)
  3. Can you explain why we would need the _SERVER variables for the PWA application? Same as above...the PWA should not have to use the same request/response object as the rest of the Magento application!
zetlen commented 4 years ago

@tjwiebell Has most of this PR been covered in #19 and this can be closed, or is there other stuff to excavate?

tjwiebell commented 4 years ago

@zetlen There is some logic remaining about automatic environment variable setting, but this is what gave us pause about merging this to begin with. I propose closing and asking @dheesbeen to open a new PR with that functionality, and justification for their use cases, and it can be reviewed during our routine grooming ceremony.