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

Adds warning if private properties are accessed outside its class. #2619

Merged
merged 2 commits into from
Aug 18, 2023

Conversation

krutidugade
Copy link
Contributor

@krutidugade krutidugade commented Aug 16, 2023

Changes proposed in this Pull Request:

We defined some private properties to eliminate dynamic property warnings in #2606. In the recent dev call, the team discussed and decided to display a warning if the private properties are accessed outside its class. This PR displays the warning with help of __get and _doing_it_wrong functions.

Related to #2606.

  • Do the changed files pass phpcs checks? Please remove phpcs:ignore comments in changed files and fix any issues, or delete if not practical.

Detailed test instructions:

  1. Checkout this branch
  2. Set WP_DEBUG and WP_DEBUG_LOG to true
  3. Access private properties by adding the following block of code under admin_notices() function in facebook-for-woocommerce.php file
$wcprod = new WC_Product();
$accessprivate = new WC_Facebook_Product($wcprod->get_id());
print_r('Hello1');
print_r($accessprivate->fb_description);
print_r($accessprivate->gallery_urls);
print_r($accessprivate->fb_use_parent_image);
print_r($accessprivate->main_description);
print_r($accessprivate->sync_short_description); 
print_r('Hello2');
$fbcom = new WC_Facebookcommerce();
print_r($fbcom->configuration_detection);
print_r($fbcom->fb_categories);
print_r('Hello3');
$integ = new WC_Facebookcommerce_Integration($fbcom);
print_r($integ->events_tracker);
print_r($integ->messenger_chat);
print_r($integ->background_processor);
print_r('Hello4');
print_r((new WooCommerce\Facebook\Admin)->product_sets);
print_r('Hello5');
print_r((new WooCommerce\Facebook\ProductSync\ProductValidator($integ, $wcprod))->facebook_product);
print_r('Hello6');
print_r((new WC_Facebookcommerce_Background_Process($integ))->commerce);
print_r('Hello7');
$enh = new WooCommerce\Facebook\Admin\Enhanced_Catalog_Attribute_Fields("Page");
print_r($enh->page_type);
print_r($enh->term);
print_r($enh->product);
print_r($enh->category_handler);
print_r('Hello8');
$settings = [
	'fb_page_id' => 'abc123',
	'facebook_jssdk_version' => '1.2.3',
];
$messen = new WC_Facebookcommerce_MessengerChat($settings);
print_r($messen->page_id);
print_r($messen->jssdk_version);
  1. Verify you see the PHP notices in debug.log and in header of admin page.
[17-Aug-2023 14:49:40 UTC] PHP Notice:  Function __get was called <strong>incorrectly</strong>. The fb_description property is private and should not be accessed outside its class. Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version x.x.x.) in /var/www/html/wp-includes/functions.php on line 5905
[17-Aug-2023 14:49:40 UTC] PHP Notice:  Function __get was called <strong>incorrectly</strong>. The gallery_urls property is private and should not be accessed outside its class. Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version x.x.x.) in /var/www/html/wp-includes/functions.php on line 5905

Changelog entry

Tweak - Displays warnings on accessing private/protected properties incorrectly.

@github-actions github-actions bot added the changelog: tweak Small change, that isn't actually very important. label Aug 16, 2023
@krutidugade krutidugade marked this pull request as ready for review August 17, 2023 15:44
@krutidugade krutidugade requested review from ibndawood and a team August 17, 2023 15:44
Copy link
Contributor

@ibndawood ibndawood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @krutidugade, for the changes. They look good to me. I've added small nitpicks but they are not a blocker.

includes/Admin.php Outdated Show resolved Hide resolved
includes/ProductSync/ProductValidator.php Outdated Show resolved Hide resolved
@rawdreeg
Copy link
Contributor

Shouldn't we add a getter and fix the warning in our own code base? That can be another PR.

@krutidugade
Copy link
Contributor Author

krutidugade commented Aug 18, 2023

Shouldn't we add a getter and fix the warning in our own code base? That can be another PR.

I agree with you. There is some room for improvement with the notice. As you suggested, we can look into it in a different PR. Currently, we'll merge this PR to proceed with the release.

@krutidugade krutidugade merged commit a9a5d85 into develop Aug 18, 2023
5 checks passed
@krutidugade krutidugade deleted the tweak/private-property-access-notice branch August 18, 2023 09:28
@krutidugade krutidugade self-assigned this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: tweak Small change, that isn't actually very important.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants