Skip to content

Conversation

@jsirish
Copy link
Member

@jsirish jsirish commented Nov 18, 2025

Upgrades the module to SilverStripe 6 compatibility.

Changes

  • Updated PHP requirement to ^8.1
  • Updated core dependency silverstripe/recipe-core from ^5 to ^6
  • Updated dynamic/silverstripe-country-dropdown-field from ^2 to ^3 (SS6 compatible)
  • Fixed namespace changes:
    • Extension: SilverStripe\ORM\DataExtensionSilverStripe\Core\Extension
  • Removed parent::onBeforeWrite() call (Extension base class doesn't have this method)
  • Changed onBeforeWrite() visibility to protected (SS6 standard)
  • Removed branch-alias

Testing

  • ✅ PHPUnit: 6/6 tests passing
  • ✅ PHPCS: Clean
  • ✅ Code follows SS6 Extension patterns

This prepares the module for the 4.0.0 release and resolves dependency issues blocking other Dynamic modules from upgrading to SS6.

- Update PHP requirement to ^8.1
- Update silverstripe/recipe-core to ^6
- Update country-dropdown-field to ^3 (SS6 compatible)
- Fix Extension namespace (ORM\DataExtension → Core\Extension)
- Remove parent::onBeforeWrite() call
- Change onBeforeWrite() to protected visibility
- Set branch-alias to 4.0.x-dev
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the module to SilverStripe 6 compatibility by updating core dependencies and adapting to SS6 Extension patterns.

  • Updated core framework dependency from silverstripe/recipe-core ^5 to ^6
  • Migrated from DataExtension to Extension base class following SS6 namespace changes
  • Adjusted onBeforeWrite() hook to protected visibility and removed obsolete parent call

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/AddressDataExtension.php Changed base class from DataExtension to Extension, updated onBeforeWrite to protected visibility, removed parent call
src/DistanceDataExtension.php Changed base class from DataExtension to Extension
composer.json Updated PHP requirement to ^8.1, silverstripe/recipe-core to ^6, country-dropdown-field to ^3, and branch-alias to 4.0.x-dev
Comments suppressed due to low confidence (2)

src/AddressDataExtension.php:23

  • The @Property annotation incorrectly documents $owner as type AddressDataExtension. Since this is an Extension, the $owner property represents the DataObject being extended, not the extension itself. This should document the actual owner class type (e.g., DataObject or the specific class this extension is applied to) or be removed entirely if the owner type varies.
 * @property AddressDataExtension $owner

src/AddressDataExtension.php:346

  • The onBeforeWrite() method lacks test coverage for the geocoding behavior it provides. This is a critical functionality that automatically updates latitude and longitude coordinates. Consider adding tests to verify that coordinates are correctly set when an address is provided, and that geocoding is skipped when LatLngOverride is true or disable_geocoding is set.
    protected function onBeforeWrite()
    {
        if ($this->hasAddress() && !$this->owner->config()->get('disable_geocoding')
            && Config::inst()->get(GoogleGeocoder::class, 'geocoder_api_key')) {
            if (!$this->isAddressChanged()) {
                return;
            }
            if ($this->owner->LatLngOverride) {
                return;
            }
            if ($address = $this->getFullAddress()) {
                $geocoder = new GoogleGeocoder($address);
                $response = $geocoder->getResult();
                $this->owner->Lat = $response->getCoordinates()->getLatitude();
                $this->owner->Lng = $response->getCoordinates()->getLongitude();
            }
        }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

/**
* Class \Dynamic\SilverStripeGeocoder\DistanceDataExtension
*
* @property DistanceDataExtension $owner
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The @Property annotation incorrectly documents $owner as type DistanceDataExtension. Since this is an Extension, the $owner property represents the DataObject being extended, not the extension itself. This should document the actual owner class type (e.g., DataObject or the specific class this extension is applied to) or be removed entirely if the owner type varies.

Suggested change
* @property DistanceDataExtension $owner

Copilot uses AI. Check for mistakes.
- Remove incorrect @Property annotation for $owner
- Add explicit nullable type hint to augmentSQL() parameter
- Update country-dropdown-field to ^3 (SS6 compatible)
- Update recipe-testing to ^4 for SS6
- Fixes PHP 8.1 deprecation warning
@jsirish jsirish requested a review from Copilot November 19, 2025 01:04
@jsirish jsirish merged commit f4411d7 into master Nov 19, 2025
16 checks passed
@jsirish jsirish deleted the 4.0/ss6-upgrade branch November 19, 2025 01:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


use SilverStripe\Core\Config\Config;
use SilverStripe\ORM\DataExtension;
use SilverStripe\Core\Extension;
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This should be use SilverStripe\ORM\DataExtension; instead of use SilverStripe\Core\Extension;.

This extension is designed to extend DataObjects (as shown in the README and TestLocation.php), and uses DataObject-specific hooks like augmentSQL(). The augmentSQL() method is only available in DataExtension, not in the base Extension class.

In SilverStripe 6, SilverStripe\ORM\DataExtension still exists and should be used for DataObject extensions. The base SilverStripe\Core\Extension is only for extending non-DataObject classes.

Copilot uses AI. Check for mistakes.
use SilverStripe\Forms\ReadonlyField;
use SilverStripe\Forms\TextField;
use SilverStripe\ORM\DataExtension;
use SilverStripe\Core\Extension;
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This should be use SilverStripe\ORM\DataExtension; instead of use SilverStripe\Core\Extension;.

This extension is designed to extend DataObjects (as shown in the README and TestLocation.php), and uses DataObject-specific hooks like onBeforeWrite(). In SilverStripe 6, SilverStripe\ORM\DataExtension still exists and should be used for DataObject extensions.

Copilot uses AI. Check for mistakes.
* @property float $Lng
*/
class AddressDataExtension extends DataExtension
class AddressDataExtension extends Extension
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This should be class AddressDataExtension extends DataExtension instead of extends Extension.

The onBeforeWrite() method (line 329) and other DataObject extension features used in this class require extending SilverStripe\ORM\DataExtension, not the base Extension class.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants