Remove usage of obsolete property $_isScopePrivate #31754

mrtuvn · 2021-01-19T07:18:24Z

Description (*)

In old codebase developers still use this property in someplace. We should clean that to follow best practices
https://devdocs.magento.com/guides/v2.4/extension-dev-guide/cache/page-caching/private-content.html#config-cache-priv-how-block
Search results return around 59 blocks still have this property. It's quite lot and can make break change if we refactor this

Just wonder if blocks created not from layout and created via other place (Controllers, Template view), How we can manager cache for that after remove isScopePrivate property

Related Pull Requests

Fixed Issues (if relevant)

Fixes More than core 80 blocks use _isScopePrivate in spite of being officially described as _not working properly_ #30506

Manual testing scenarios (*)

...
...

Questions or comments

CC: @ihor-sviziev @ericmorand @marvinhinz @orlangur @sidolov
Feel free to give me any comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2021-01-19T07:18:26Z

Hi @mrtuvn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.5-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

ihor-sviziev · 2021-01-19T09:47:19Z

TBH I don't think it's a bad practice to use _isScopePrivate. It would be good to understand what issues it's causing.
The cacheable=false causing the entire page to be not cached at all.

mrtuvn · 2021-01-19T10:16:47Z

We should update more in docs related with this. Maybe add some real-world use case for example. Seem currently it's not clearly at my point of view

ihor-sviziev · 2021-01-19T10:50:47Z

It seems like a message about not using _isScopePrivat was added a long time ago, in 2016 magento/devdocs@a75a7a9#diff-21ef8286af3f0fb99ca6976a2c50f78693f2fe7e46b6394635ac8112792087adR62-R64.

@sivaschenko @sidolov, could you explain why this approach was deprecated and should work incorrectly but still used in the core?

sivaschenko · 2021-01-19T12:10:42Z

@ihor-sviziev I believe the reason to walk away from $_isScopePrivate is to handle and invalidate private data more precisely that is possible with customer data sections.

The property was not yet deprecated by core teams due to low priority, internal ticket: https://jira.corp.magento.com/browse/MAGETWO-56701

sivaschenko · 2021-01-19T12:11:43Z

This pull request not fixing but is related to the issue #30506 (just to create a link)

sivaschenko · 2021-01-19T12:13:54Z

I'd probably try to deprecate the property in 2.4 and remove in 2.5, @gabrieldagama @sidolov what do you think?

mrtuvn · 2021-01-19T12:41:53Z

Should i re-target branch and update issue link ?

sivaschenko · 2021-01-19T14:14:23Z

@mrtuvn considering the tumb-ups, yes, please

Thanks! :)

sivaschenko · 2021-01-19T14:18:18Z

app/code/Magento/Multishipping/Block/Checkout/Address/Select.php

-    /**
-     * @var bool
-     */
-    protected $_isScopePrivate = true;


While it should be ok to remove the redeclarations of the inherited property with the same value, I don't think we should remove the $_isScopePrivate = true in this pull request (that should be done in 2.5 as part as deprecated property removal preparation)

should i need revert all that ? only keep deprecate notes

This is the only file where the removal should be reverted

sivaschenko · 2021-01-19T14:19:57Z

Prioritized as P2 as missing the deprecated tag on the property prevents any further progress in moving to best practices in this direction

ihor-sviziev · 2021-01-19T15:01:44Z

@magento run all tests

magento-engcom-team · 2021-01-20T06:54:44Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8668 has been created to process this Pull Request

engcom-Bravo · 2021-01-20T12:14:54Z

✔️ QA Passed

Manual testing scenario

Open Magento project in PHP Storm, for example
Go to /app/code/Magento
Search for text $this->_isScopePrivate = true

AFTER applying changes provided in this PR, all the $this->_isScopePrivate = true are marked as Deprecated
see an example

up

mrtuvn · 2021-01-21T14:54:35Z

Only rebase code nothing else change

engcom-Foxtrot · 2021-01-22T08:37:12Z

@magento run all tests

engcom-Bravo · 2021-01-22T11:06:54Z

✔️ QA Passed
The result is the same as in the report above #31754 (comment)

mrtuvn · 2021-01-23T09:48:59Z

@magento run Functional Tests B2B

m2-assistant · 2021-01-31T16:23:58Z

Hi @mrtuvn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

m2-community-project bot added the Progress: pending review label Jan 19, 2021

mrtuvn force-pushed the get-rid-off-old-property-scope-private branch 2 times, most recently from d566665 to 07438f7 Compare January 19, 2021 10:14

mrtuvn force-pushed the get-rid-off-old-property-scope-private branch from 07438f7 to 382d77f Compare January 19, 2021 10:29

mrtuvn changed the base branch from 2.5-develop to 2.4-develop January 19, 2021 14:13

mrtuvn marked this pull request as ready for review January 19, 2021 14:14

sivaschenko reviewed Jan 19, 2021

View reviewed changes

m2-community-project bot assigned sivaschenko Jan 19, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels Jan 19, 2021

sivaschenko added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jan 19, 2021

mrtuvn force-pushed the get-rid-off-old-property-scope-private branch 3 times, most recently from 89d3a2c to ffc0d75 Compare January 19, 2021 15:00

ihor-sviziev assigned ihor-sviziev and sivaschenko and unassigned sivaschenko Jan 19, 2021

m2-community-project bot added Progress: pending review and removed Progress: review labels Jan 19, 2021

magento-engcom-team added Release Line: 2.4 Component: View labels Jan 20, 2021

engcom-Bravo self-assigned this Jan 20, 2021

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Jan 20, 2021

m2-community-project bot added Progress: extended testing and removed Progress: testing in progress labels Jan 20, 2021

Update note deprecated property $_isScopePrivate

f340589

up

mrtuvn force-pushed the get-rid-off-old-property-scope-private branch from ffc0d75 to f340589 Compare January 21, 2021 14:54

engcom-Foxtrot self-assigned this Jan 22, 2021

m2-community-project bot added Progress: ready for testing and removed Progress: extended testing labels Jan 22, 2021

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Jan 22, 2021

m2-community-project bot added Progress: extended testing and removed Progress: testing in progress labels Jan 22, 2021

m2-community-project bot added Progress: accept and removed Progress: extended testing labels Jan 25, 2021

magento-engcom-team pushed a commit that referenced this pull request Jan 31, 2021

ENGCOM-8668: Remove usage of obsolete property $_isScopePrivate #31754

8017178

magento-engcom-team merged commit 940a54c into magento:2.4-develop Jan 31, 2021

mrtuvn deleted the get-rid-off-old-property-scope-private branch February 5, 2021 14:46

gabrieldagama added the Type: Code Cleanup label Mar 2, 2021

onlinebizsoft mentioned this pull request Jul 22, 2021

_isScopePrivate vs ESI hole punching vs private data #33555

Closed

5 tasks

Remove usage of obsolete property $_isScopePrivate #31754

Remove usage of obsolete property $_isScopePrivate #31754

Uh oh!

Conversation

mrtuvn commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jan 19, 2021

Uh oh!

ihor-sviziev commented Jan 19, 2021

Uh oh!

mrtuvn commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihor-sviziev commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sivaschenko commented Jan 19, 2021

Uh oh!

sivaschenko commented Jan 19, 2021

Uh oh!

sivaschenko commented Jan 19, 2021

Uh oh!

mrtuvn commented Jan 19, 2021

Uh oh!

sivaschenko commented Jan 19, 2021

Uh oh!

sivaschenko Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

mrtuvn Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

sivaschenko Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivaschenko commented Jan 19, 2021

Uh oh!

ihor-sviziev commented Jan 19, 2021

Uh oh!

magento-engcom-team commented Jan 20, 2021

Uh oh!

engcom-Bravo commented Jan 20, 2021

Manual testing scenario

Uh oh!

mrtuvn commented Jan 21, 2021

Uh oh!

engcom-Foxtrot commented Jan 22, 2021

Uh oh!

engcom-Bravo commented Jan 22, 2021

Uh oh!

mrtuvn commented Jan 23, 2021

Uh oh!

m2-assistant bot commented Jan 31, 2021

Uh oh!

Uh oh!

mrtuvn commented Jan 19, 2021 •

edited

Loading

mrtuvn commented Jan 19, 2021 •

edited

Loading

ihor-sviziev commented Jan 19, 2021 •

edited

Loading

sivaschenko Jan 19, 2021 •

edited

Loading