Skip to content

Deprecate ResourceInterface::getResource() #15719

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

Merged
merged 1 commit into from
Sep 26, 2015
Merged

Deprecate ResourceInterface::getResource() #15719

merged 1 commit into from
Sep 26, 2015

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Sep 7, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR n/a

The return value of this method does not make sense if you do not exactly know about the type of resource at hand. For example, it may be an array or a file path.

As all usages of getResource() within Symfony are in tests of particular Resource implementations anyway, deprecating and later removing this method helps us with simplifying the ResourceInterface (#7176).

@stof
Copy link
Member

stof commented Sep 8, 2015

I think we need to introduce a whole new resource system, which would move the checking of freshness outside the resource (it is not possible to implement such feature in the existing interface). As such, I don't think we need to deprecate part of the interface now (we should deprecate the whole interface when implementing the new system)

@mpdude
Copy link
Contributor Author

mpdude commented Sep 8, 2015

@stof please see #15692 #15738, I think we can make it with refactorings.

@stof
Copy link
Member

stof commented Sep 8, 2015

@mpdude the issue is that ResourceInterface::isFresh simply cannot be implemented for the new system, as resources don't have the necessary info to check themselves (in the general case).
This is why we need a new API, with a new interface (and the new interface should be enforced to be Serializable IMO).

@mpdude
Copy link
Contributor Author

mpdude commented Sep 8, 2015

@stof #15692 #15738 tries to shift resource validation to services which are called MetadataValidator there.

For resources implementing ResourceInterface, a generic validator can be used and it needs the isFresh method.

If we remove ResourceInterface::getResource() because we don't need it, the only requirement for resources in the general case is to implement __toString which is needed for de-duplication.

So, we could extract that into a base interface (ResourceMetadata, CacheMetadata or ...?) and make this base interface the only requirement for resources.

@stof
Copy link
Member

stof commented Sep 8, 2015

@mpdude a resource should also be serializable, as we serialize it in the meta file. This is why I think we need a new interface (freeing us from BC on the interface itself), with an implementation wrapping the existing ResourceInterface.

@mpdude
Copy link
Contributor Author

mpdude commented Sep 8, 2015

Is it really necessary to extend \Serializable? What difference does it make?

@stof
Copy link
Member

stof commented Sep 8, 2015

@mpdude makig sure resources are actually serializable (people broke things in the past when putting a PDO instance in their own resources as this would not make them serializable anymore)

@mpdude
Copy link
Contributor Author

mpdude commented Sep 8, 2015

Yes, but they will find out rather quickly and still can implement \Serializable, not?

For most simple resources that work fine with the built-in serialization mechanism, I'd like to avoid the hassle of having to implement __sleep and __wakeup. Plus it's not a BC issue on ResourceInterface which works without Serializable right now, and changing that is hard (or even impossible).

@stof
Copy link
Member

stof commented Sep 8, 2015

@mpdude if you don't implement Serializable, it also means you are not allowed to modify your class anymore, because it would break BC for the unserialization of existing meta. This is why core resources are serializable btw (and also to reduce the size of the serialized data)

@mpdude
Copy link
Contributor Author

mpdude commented Sep 24, 2015

ping @fabpot / @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Sep 25, 2015

needs a rebase

The return value of this method does not make sense if you do not exactly know
about the type of resource at hand. For example, it may be [an array](https://github.com/symfony/symfony/blob/b49fa129bdb3c0aa970a006b16dd1ca63a9d7ebd/src/Symfony/Component/HttpKernel/Config/EnvParametersResource.php#L57)
or a [file path](https://github.com/symfony/symfony/blob/87800ae47e64429f2544b798575d1cc1d4e5464a/src/Symfony/Component/Config/Resource/FileResource.php#L51).

As all usages of getResource() within Symfony are in tests of particular Resource implementations anyway, deprecating and later removing this interface
helps us with simplifying the ResourceInterface (#7176).
@mpdude
Copy link
Contributor Author

mpdude commented Sep 25, 2015

Rebased.

@fabpot
Copy link
Member

fabpot commented Sep 25, 2015

Do we really need to keep this interface for 3.0?

@mpdude
Copy link
Contributor Author

mpdude commented Sep 25, 2015

We need the __toString() for resource deduplication, plus there are @api interfaces that use it for type hints.

@fabpot
Copy link
Member

fabpot commented Sep 26, 2015

Thank you @mpdude.

@fabpot fabpot merged commit 87c0c7d into symfony:2.8 Sep 26, 2015
fabpot added a commit that referenced this pull request Sep 26, 2015
This PR was merged into the 2.8 branch.

Discussion
----------

Deprecate ResourceInterface::getResource()

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | n/a

The return value of this method does not make sense if you do not exactly know about the type of resource at hand. For example, it may be [an array](https://github.com/symfony/symfony/blob/b49fa129bdb3c0aa970a006b16dd1ca63a9d7ebd/src/Symfony/Component/HttpKernel/Config/EnvParametersResource.php#L57) or a [file path](https://github.com/symfony/symfony/blob/87800ae47e64429f2544b798575d1cc1d4e5464a/src/Symfony/Component/Config/Resource/FileResource.php#L51).

As all usages of getResource() within Symfony are in tests of particular Resource implementations anyway, deprecating and later removing this method helps us with simplifying the ResourceInterface (#7176).

Commits
-------

87c0c7d Deprecate ResourceInterface::getResource()
@mpdude mpdude deleted the remove-get-resource-from-resource-interface branch September 26, 2015 08:35
fabpot added a commit that referenced this pull request Sep 28, 2015
…which was deprecated in 2.8 (mpdude)

This PR was squashed before being merged into the 3.0-dev branch (closes #15929).

Discussion
----------

[3.0][Config] Remove ResourceInterface::getResource() which was deprecated in 2.8

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Deprecated in #15719.

Commits
-------

7cef180 [3.0][Config] Remove ResourceInterface::getResource() which was deprecated in 2.8
@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

4 participants