bug #239 Revert #237 and deprecate extending a protected closure (ska…

…lpa)

This PR was squashed before being merged into the 3.2.x-dev branch (closes #239).

Discussion
----------

Revert #237 and deprecate extending a protected closure

Reverts #237 and throws a deprecation notice instead of an exception when attempting to extend a protected closure.

As I said in silexphp/Silex#1538, I believe there are two ways to correctly handle the issue:

1. We throw an exception, considering that protected closures are parameters values and that extending them should be forbidden.

2. We allow users to extend anything. In that case, one should expect both these tests to work IMHO:

```php
$pimple['foo'] = 'Hello';

$pimple['bar'] = $pimple->protect(function () {
    return 'Hello';
});

$pimple->extend('foo', function ($helloString) {
    return $helloString.' World';
});

$pimple->extend('bar', function ($helloClosure) {
    return $helloClosure().' World';
});

$this->assertSame('Hello World', $pimple['foo']);
$this->assertSame('Hello World', $pimple['bar']);
```

Now, considering that Pimple invokes protected closures when the service is initialized, the current behavior forces you to do this instead (it basically ignores that the closure is protected and treats it as a regular, unprotected entry):

```php
$pimple['bar'] = $pimple->protect(function () {
    return 'Hello';
});

$pimple->extend('bar', function ($helloString) {
    return $helloString.' World';
});
```

That means both fixes would break BC, so I reverted #237 and added a deprecation notice instead.

(PHP implementation is fixed, C is coming up)

Commits
-------

9bcdb4d Revert #237 and deprecate extending a protected closure

ext/pimple/pimple.c

@@ -715,10 +715,16 @@ PHP_METHOD(Pimple, extend)
 				RETURN_NULL();
 			}
 
-			if (value->type != PIMPLE_IS_SERVICE || zend_hash_index_exists(&pobj->protected, value->handle_num)) {
+			if (value->type != PIMPLE_IS_SERVICE) {
 				pimple_throw_exception_string(pimple_ce_InvalidServiceIdentifierException, Z_STRVAL_P(offset), Z_STRLEN_P(offset) TSRMLS_CC);
 				RETURN_NULL();
 			}
+			if (zend_hash_index_exists(&pobj->protected, value->handle_num)) {
+				int er = EG(error_reporting);
+				EG(error_reporting) = 0;
+				php_error(E_DEPRECATED, "How Pimple behaves when extending protected closures will be fixed in Pimple 4. Are you sure \"%s\" should be protected?", Z_STRVAL_P(offset));
+				EG(error_reporting) = er;
+			}
 		break;
 		case IS_DOUBLE:
 		case IS_BOOL:
@@ -733,11 +739,17 @@ PHP_METHOD(Pimple, extend)
 				pimple_throw_exception_string(pimple_ce_UnknownIdentifierException, Z_STRVAL_P(offset), Z_STRLEN_P(offset) TSRMLS_CC);
 				RETURN_NULL();
 			}
-			if (value->type != PIMPLE_IS_SERVICE || zend_hash_index_exists(&pobj->protected, value->handle_num)) {
+			if (value->type != PIMPLE_IS_SERVICE) {
 				convert_to_string(offset);
 				pimple_throw_exception_string(pimple_ce_InvalidServiceIdentifierException, Z_STRVAL_P(offset), Z_STRLEN_P(offset) TSRMLS_CC);
 				RETURN_NULL();
 			}
+			if (zend_hash_index_exists(&pobj->protected, value->handle_num)) {
+				int er = EG(error_reporting);
+				EG(error_reporting) = 0;
+				php_error(E_DEPRECATED, "How Pimple behaves when extending protected closures will be fixed in Pimple 4. Are you sure \"%ld\" should be protected?", index);
+				EG(error_reporting) = er;
+			}
 		break;
 		case IS_NULL:
 		default:

src/Pimple/Container.php

@@ -241,10 +241,14 @@ public function extend($id, $callable)
             throw new FrozenServiceException($id);
         }
 
-        if (!is_object($this->values[$id]) || !method_exists($this->values[$id], '__invoke') || isset($this->protected[$this->values[$id]])) {
+        if (!is_object($this->values[$id]) || !method_exists($this->values[$id], '__invoke')) {
             throw new InvalidServiceIdentifierException($id);
         }
 
+        if (isset($this->protected[$this->values[$id]])) {
+            @trigger_error(sprintf('How Pimple behaves when extending protected closures will be fixed in Pimple 4. Are you sure "%s" should be protected?', $id), E_USER_DEPRECATED);
+        }
+
         if (!is_object($callable) || !method_exists($callable, '__invoke')) {
             throw new ExpectedInvokableException('Extension service definition is not a Closure or invokable object.');
         }

src/Pimple/Tests/PimpleTest.php

@@ -392,17 +392,21 @@ public function testLegacyExtendFailsForKeysNotContainingServiceDefinitions($ser
     }
 
     /**
-     * @expectedException \Pimple\Exception\InvalidServiceIdentifierException
-     * @expectedExceptionMessage Identifier "foo" does not contain an object definition.
+     * @group legacy
+     * @expectedDeprecation How Pimple behaves when extending protected closures will be fixed in Pimple 4. Are you sure "foo" should be protected?
      */
-    public function testExtendFailsIfEntryIsProtected()
+    public function testExtendingProtectedClosureDeprecation()
     {
         $pimple = new Container();
         $pimple['foo'] = $pimple->protect(function () {
             return 'bar';
         });
 
-        $pimple->extend('foo', function () {});
+        $pimple->extend('foo', function ($value) {
+            return $value.'-baz';
+        });
+
+        $this->assertSame('bar-baz', $pimple['foo']);
     }
 
     /**