Part of #2492955: [meta] PSR-7 support

This issue is just to add the dependent libraries to Drupal core and register them with the container.

https://packagist.org/packages/symfony/psr-http-message-bridge
https://packagist.org/packages/zendframework/zend-diactoros

In addition to adding the composer libraries we also should register the two factory services with the container: HttpFoundationFactory and DiactorosFactory.

See the parent issue for the larger intent.

This issue is nominally blocked on #2470693: Upgrade to Symfony 2.7.0 but only for reroll-difficulty reasons.

CommentFileSizeAuthor
#31 include_symfony_psr_7-2497691-31.patch264.89 KBcilefen
#31 include_symfony_psr_7-2497691-31-do-not-test.patch7.37 KBcilefen
#28 2497691-psr7-do-not-test.patch7.37 KBCrell
#28 2497691-psr7.patch265.23 KBCrell
#24 interdiff-2497691.txt632 bytesCrell
#24 2497691-psr7-do-not-test.patch7.37 KBCrell
#24 2497691-psr7.patch263.33 KBCrell
#19 2497691-psr7-do-not-test.patch7.38 KBCrell
#19 2497691-psr7.patch263.34 KBCrell
#17 2497691-psr7-do-not-test.patch6.78 KBCrell
#17 2497691-psr7.patch263.32 KBCrell
#6 include_symfony_psr_7-2497691-6.patch261.58 KBcilefen
#6 include_symfony_psr_7-2497691-6-DRUPAL-ONLY.patch8.06 KBcilefen
#6 interdiff.txt3.53 KBcilefen
#3 psr-7-bridge-library-2497691-1.patch1.28 KBmarvin_b8

Comments

Crell’s picture

Crell commented
Issue summary: View changes
cilefen’s picture

cilefen commented
Status: Active » Needs review
StatusFileSize
new include_symfony_psr_7-2497691-2.patch261.94 KB

I am not sure what to name those services.

marvin_b8’s picture

marvin_b8 commented
StatusFileSize
new psr-7-bridge-library-2497691-1.patch1.28 KB

We have a problem with the php version requirements of drupal and zend diactoros.

https://packagist.org/packages/zendframework/zend-diactoros
php: >=5.5

Drupal 8: PHP 5.4.5 or higher

its work for me but ....

Status: Needs review » Needs work

The last submitted patch, 3: psr-7-bridge-library-2497691-1.patch, failed testing.

Crell’s picture

Crell commented 
  1. +++ b/core/composer.json
@@ -13,6 +13,7 @@
+    "symfony/psr-http-message-bridge": "dev-master",

    There is a tagged 0.1 release. Why not use that? 

  2. +++ b/core/core.services.yml
@@ -597,6 +597,10 @@ services:
+  diactoros.factory:

    Maybe call this psr7.factory, to avoid the silly name? Hm. Open to suggestions here.

Can you include a diff in the future that is just the changes outside of vendor? It makes it much easier to review. We'll commit the patch with vendor but it's a lot nicer to review one without. Thanks!

cilefen’s picture

cilefen commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.53 KB
new include_symfony_psr_7-2497691-6-DRUPAL-ONLY.patch8.06 KB
new include_symfony_psr_7-2497691-6.patch261.58 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/composer.json
@@ -13,6 +13,7 @@
+    "symfony/psr-http-message-bridge": "v0.1",

So does symfony promises similar APIs even before 1.0?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Crell in #2492955: [meta] PSR-7 support you said you nudged the maintianers of this to make it php 5.4 compatible - did you create a PR? If so is there a link?

Crell’s picture

Crell commented

No PR, but a Twitter discussion: https://twitter.com/Crell/status/604787960127582208 (summary: MWOP says they'll think about it)

I also asked the Symfonians about stability of the bridge library's API: https://twitter.com/Crell/status/605124382696955904 (summary: Shouldn't change, esp. since it's such a small lib, but even if it does stable should be out before 8.0.0 anyway and it needn't be public facing for our user base)

(Feels weird to be posting Twitter threads to d.o as documentation, but welcome to the age of social media...)

Crell queued 6: include_symfony_psr_7-2497691-6.patch for re-testing.

Crell’s picture

Crell commented

Does anyone else have thoughts on the service names, while we're waiting on confirmation from Alex?

Status: Needs review » Needs work

The last submitted patch, 6: include_symfony_psr_7-2497691-6.patch, failed testing.

cilefen’s picture

cilefen commented

#2497693: Add PSR-7 to Symfony Response View listener has:

symfony.psr7.http_foundation_factory

So in that paradigm we would have:

zend.psr7.diactoros_factory

Crell’s picture

Crell commented

We're not namespacing by the 3rd party library anywhere else. I don't see a reason to do so here.

Crell’s picture

Crell commented
Assigned: Unassigned » alexpott

alexpott: https://github.com/zendframework/zend-diactoros/pull/48

PR there to drop the version requirement for Diactoros, at least for the time being. Assuming that's merged, the next release would be compatible with us with no additional version issues. If that unblocks moving forward, it would be good to mention that on the thread to encourage MWOP to commit it sooner rather than later. :-)

If green lit, we just need a reroll here and a verdict on the service names. (Alex, I defer to you if you have any strong preferences.)

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/core.services.yml
@@ -597,6 +597,10 @@ services:
+  psr7_foundation.factory:
+    class: Symfony\Bridge\PsrHttpMessage\Factory\HttpFoundationFactory
+  psr7_diactoros.factory:
+    class: Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory

I'd prefer more something like request_from_psr7.factory psr7_from_request.factory ... which library we use is a detail IMHO and might be different in the future

Crell’s picture

Crell commented
Status: Needs work » Needs review
StatusFileSize
new 2497691-psr7.patch263.32 KB
new 2497691-psr7-do-not-test.patch6.78 KB

Rerolling, with new service names similar to #16.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/core.services.yml
@@ -596,6 +596,10 @@ services:
+  psr7_request.factory:
...
+  request_psr7.factory:

This looks superconfusing to me... I guess (and it only a guess) that psr7_request.factory converts a PSR7 request to a Symfony request and request_psr7.factory does it the other way.

I think psr7_to_symfony.request.factory and symfony_to_psr7.request.factory is better since it is more explicit.

Crell’s picture

Crell commented
StatusFileSize
new 2497691-psr7.patch263.34 KB
new 2497691-psr7-do-not-test.patch7.38 KB

Not my favorite names, but eh, I'd rather get the libraries in. :-)

New patches per #18. I forgot to make an interdiff before rebasing, but the only change is the service rename per #18.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Crell names here are important so let's try and work to towards a consensus. My problem with psr7_request/request_psr7 is that a PSR7 request is a thing so I would expect psr7_request.factory to produce psr7 requests but it does not to. It converts psr7 requests into a Symphony request object.

Crell’s picture

Crell commented

Well, my issue is more that service names feel like they should be nouns. "foo_to_bar" et al feels more like a verb (method name). Dropping the "to" in the middle was the best suggestion I had though.

I've pinged a Symfonian for their input, but worst case scenario we have somewhat odd names for 2 services that rarely will any module developers touch directly. I just don't want to get this set of issues hung up on a bikeshed. :-)

fabianx’s picture

fabianx commented

Those are converter services, so why not something like:

converter.psr7_request.factory
converter.request_psr7.factory

Not sure we need even the factory specification (without looking at the patch):

converter.psr7_to_request
converter.request_to_psr7

or

converter.psr7_request
converter.request_psr7

Thoughts?

Crell’s picture

Crell commented

Data point: In Symfony 2.7, the services are registered via the SensioFrameworkExtraBundle. Its service names are:

sensio_framework_extra.psr7.http_message_factory
sensio_framework_extra.psr7.http_foundation_factory

Obviously we don't need the first part, but what about:

psr7.http_message_factory
psr7.http_foundation_factory

(The first producing PSR7 messages, the second producing HttpFoundation classes.)

(HT to Ryan Weaver for linking me to those.)

Crell’s picture

Crell commented
StatusFileSize
new 2497691-psr7.patch263.33 KB
new 2497691-psr7-do-not-test.patch7.37 KB
new interdiff-2497691.txt632 bytes

I am going to take silence as approval, so here's a version with the service names from #23.

Can haz commits? Or RTBCz?

dawehner’s picture

dawehner
Primary language German
commented

+1

cilefen’s picture

cilefen commented

I am not sold on the meaning of "factory" in the service names.

dawehner’s picture

dawehner
Primary language German
commented

The helpful thing of putting factory in there is that it actually explains, what its doing, which is IMHO an important piece of the puzzle.

Crell’s picture

Crell commented
StatusFileSize
new 2497691-psr7.patch265.23 KB
new 2497691-psr7-do-not-test.patch7.37 KB

A new version of Diactoros was just released. Rerolling for that. No other changes.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Alright, let's see whether the POST request makes it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2497691-psr7.patch, failed testing.

cilefen’s picture

cilefen commented
Status: Needs work » Needs review
StatusFileSize
new include_symfony_psr_7-2497691-31-do-not-test.patch7.37 KB
new include_symfony_psr_7-2497691-31.patch264.89 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #27

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Okay - let's join the PSR-7 revolution :) Drupal 8 has had a long term goal to be interoperable with other frameworks as pointed out by @yched in the meta issue:

PSR-7 sounds very promising in terms of ecosystem interoperability, would be awesome to have D8 support it out of the box.

Given the lack of disruption and the avenues for this opens up, committed a9dd72b and pushed to 8.0.x. Thanks!

  • alexpott committed a9dd72b on 8.0.x 
    Issue #2497691 by Crell, cilefen, marvin_B8, dawehner: Include Symfony...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.