Skip to content
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

Remove target page code #1482

Merged
merged 1 commit into from Jul 22, 2022
Merged

Conversation

eerison
Copy link
Contributor

@eerison eerison commented Jul 21, 2022

Remove old target page code

I am targeting this branch, because the target page need to be deprecated in 3.x first.

Issue #1481 .

Changelog

### Deprecated
- Deprecated target page code from `src/Command/CloneSiteCommand.php`
- Deprecated target page code from `src/Entity/Transformer.php`
- Deprecated `src/Model/Page::target` property
- Deprecated `src/Model/Page::getTarget()` method
- Deprecated `src/Model/Page::setTarget()` method
- Deprecated `src/Model/PageInterface::getTarget()` method
- Deprecated `src/Model/PageInterface::setTarget()` method
- Deprecated `src/Model/Snapshot::target` property
- Deprecated `src/Model/Snapshot::targetId` property
- Deprecated `src/Model/Snapshot::getTarget()` method
- Deprecated `src/Model/Snapshot::setTarget()` method
- Deprecated `src/Model/Snapshot::getTargetId()` method
- Deprecated `src/Model/Snapshot::setTargetId()` method
- Deprecated `src/Model/SnapshotPageProxy::target` property
- Deprecated `src/Model/SnapshotPageProxy::getTargetId()` method
- Deprecated `src/Model/SnapshotPageProxy::setTargetId()` method
- Deprecated `src/Page/PageServiceManager::createResponse()` method

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

It was the places that I found where targetId is set, But it'll be executed only if exist target page

and I didn't find any where to set this target_id

even in the admin
Screenshot 2022-07-21 at 13 29 16

then my thought is, it's not used 👀

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

@VincentLanglet and @jordisala1991 I know you didn't know the project, But in every place that uses this target_id, to be set, it depends that target already exist, But as never it will be set, then it'll never it'll be used!

the question is, should I deprecate in 3.x and remove in 4.x what do you think?

Note:

I checked in 2 projects that uses sonata page, and any of them uses this target_id

checked

bin/console doctrine:query:sql "select target_id, content from page__snapshot where target_id is not null or content not like '%target_id\":null%'"

bin/console doctrine:query:sql "select target_id, content from page__snapshot where target_id is not null"

both queries returned empty.

Edit 1: I want to confirm and know your opinion before invest time on that!

@jordisala1991
Copy link
Member

Some of that logic was added 10 years ago, here: fc9cd60 but seems incomplete...

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

Some of that logic was added 10 years ago, here: fc9cd60 but seems incomplete...

well if it's not used, i would remove it, it will help us to have less code to maintain

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

Ok I got a new update, there is a really old project that uses ~3.7 version of sonata page, and in that project this target_id is used, I don't know if it was migrated from another old version like 2.x, But there are some pages with target_id.

Edit 1: probably this comes from 2.x version look: https://github.com/sonata-project/SonataPageBundle/blob/2.x/Admin/PageAdmin.php#L208, in the 2.x version was possible to add target page, but it was removed on 3.x

Edit 2: I guess here was the mistake/feature 😄 , it was changed from target to parent, and there are 2 parents fields in the code.

Edit 3: It was introduced in 3.5.1 and in 3.6.0 branch the file was removed 👀

Edit 4: I colected some information and this feature was used for

exactly that was the selling point to this field if you have the same content but on different urls just create it once then use this field for the second one.

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

the feature was just removed, then I guess it's fine we remove the code related with this, don't you?
if someone need this feature again, he can provider a PR for this, what do you think?

@jordisala1991
Copy link
Member

So the feature was to kind of duplicate pages with same content? IMO if the feature was kind of removed and there are sone leftovers I would completely remove it, and if someone comes up with some need for it, we can always reimplement.
TBH this might be broken for a lot of years, we will not miss this code.

Lets deprecate all related to target id on 3.x and remove on 4.x. Can you do it?

@eerison
Copy link
Contributor Author

eerison commented Jul 21, 2022

So the feature was to kind of duplicate pages with same content? IMO if the feature was kind of removed and there are sone leftovers I would completely remove it, and if someone comes up with some need for it, we can always reimplement. TBH this might be broken for a lot of years, we will not miss this code.

Lets deprecate all related to target id on 3.x and remove on 4.x. Can you do it?

Yeah, let's do that way ;)

@eerison eerison changed the title Remove target_id Remove target page code Jul 22, 2022
@eerison eerison marked this pull request as ready for review July 22, 2022 08:20
@jordisala1991 jordisala1991 merged commit 01a2e04 into sonata-project:3.x Jul 22, 2022
@jordisala1991
Copy link
Member

Thanks @eerison

@eerison eerison deleted the remove_target_id branch July 22, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants