Add Interfaces to reflect DB types #5104

sreichel · 2025-11-16T19:03:47Z

Description (*)

Add interface to have ...

setter/getter for every db field
ensures input/output type matches db
less hard-coded strings ...

Related Pull Requests

@addison74 your idea to trim input/output could be added here.

Copilot

Pull Request Overview

This PR introduces data interfaces (PageInterface and BlockInterface) to standardize database field access in the CMS module, replacing hard-coded strings with constants. The changes include implementing these interfaces in the respective model classes with explicit getter/setter methods and updating all references throughout controllers, blocks, helpers, and database setup files.

Key Changes

Added two new interface files defining constants and method signatures for CMS pages and blocks
Updated model classes to implement these interfaces with typed getter/setter methods
Replaced hard-coded field name strings with interface constants across 12+ files
Enhanced documentation with missing @throws annotations

Reviewed Changes

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

Show a summary per file

File	Description
app/code/core/Mage/Cms/Api/Data/PageInterface.php	New interface defining constants and method signatures for CMS page entity
app/code/core/Mage/Cms/Api/Data/BlockInterface.php	New interface defining constants and method signatures for CMS block entity
app/code/core/Mage/Cms/Model/Page.php	Implements PageInterface with explicit getter/setter methods
app/code/core/Mage/Cms/Model/Block.php	Implements BlockInterface with explicit getter/setter methods
app/code/core/Mage/Cms/sql/cms_setup/install-1.6.0.0.php	Replaces hard-coded column names with interface constants in schema definition
app/code/core/Mage/Cms/Model/Resource/Page.php	Updates database queries to use interface constants instead of strings
app/code/core/Mage/Cms/controllers/PageController.php	Uses interface constants for request parameter access
app/code/core/Mage/Cms/Controller/Router.php	Uses interface constants for request parameter setting
app/code/core/Mage/Adminhtml/controllers/Cms/PageController.php	Updates parameter handling with interface constants
app/code/core/Mage/Adminhtml/controllers/Cms/BlockController.php	Updates parameter handling with interface constants
app/code/core/Mage/Cms/Block/Page.php	Uses interface constants and updates config path references
app/code/core/Mage/Cms/Block/Widget/Page/Link.php	Uses interface constants for data access
app/code/core/Mage/Cms/Block/Widget/Block.php	Uses interface constants for block ID access
app/code/core/Mage/Adminhtml/Block/Cms/Page/Edit/Tab/Design.php	Uses interface constants for form field names
app/code/core/Mage/Cms/controllers/IndexController.php	Added missing `@throws` documentation
app/code/core/Mage/Cms/Helper/Page.php	Added missing `@throws` documentation

app/code/core/Mage/Cms/Model/Page.php

Copilot · 2025-11-16T19:07:53Z

app/code/core/Mage/Cms/Model/Page.php

+     */
+    public function getStoreId(): ?int
+    {
+        return (int) $this->getDataByKey(self::DATA_STORE_ID);


The getStoreId() method returns ?int but doesn't handle the case where getDataByKey() returns null. The cast (int) null will return 0, not null. Consider: $storeId = $this->getDataByKey(self::DATA_STORE_ID); return is_null($storeId) ? null : (int) $storeId;

Suggested change

return (int) $this->getDataByKey(self::DATA_STORE_ID);

$storeId = $this->getDataByKey(self::DATA_STORE_ID);

return $storeId === null ? null : (int) $storeId;

Copilot · 2025-11-16T19:07:53Z

app/code/core/Mage/Cms/sql/cms_setup/install-1.6.0.0.php

+        $installer->getFkName('cms/page_store', PageInterface::DATA_STORE_ID, 'core/store', PageInterface::DATA_STORE_ID),
+        PageInterface::DATA_STORE_ID,
        $installer->getTable('core/store'),
-        'store_id',
+        PageInterface::DATA_STORE_ID,


[nitpick] Using PageInterface::DATA_STORE_ID as the foreign key column for the core/store table is incorrect. The core/store table has its own store_id column that should be referenced. While the column names happen to be the same, semantically this should use the constant from the appropriate interface for the store table, not from PageInterface.

Copilot · 2025-11-16T19:07:53Z

app/code/core/Mage/Cms/Model/Page.php

+    /**
+     * @return $this
+     */
+    public function setTitle(?string $title)
+    {
+        return $this->setData(self::DATA_TITLE, $title);
+    }


The setTitle() method is missing the @api docblock annotation, while other similar setter methods in the class have it. Consider adding @api annotation for consistency.

Copilot · 2025-11-16T19:07:54Z

app/code/core/Mage/Cms/Block/Page.php

        if (Mage::getStoreConfig('web/default/show_cms_breadcrumbs')
            && ($breadcrumbs = $this->getLayout()->getBlock('breadcrumbs'))


The variable $breadcrumbs = null; declaration on line 54 has been removed, but the variable is still used in the condition on line 58. This will cause an undefined variable error if the condition short-circuits before the assignment within the condition.

Copilot · 2025-11-16T19:07:55Z

app/code/core/Mage/Cms/Model/Block.php

+     */
+    public function getStoreId(): ?int
+    {
+        return $this->getDataByKey(self::DATA_STORE_ID);


The getStoreId() method returns ?int but doesn't handle the case where getDataByKey() returns null. The cast (int) null will return 0, not null. Consider: $storeId = $this->getDataByKey(self::DATA_STORE_ID); return is_null($storeId) ? null : (int) $storeId;

Suggested change

return $this->getDataByKey(self::DATA_STORE_ID);

$storeId = $this->getDataByKey(self::DATA_STORE_ID);

return $storeId === null ? null : (int) $storeId;

Copilot · 2025-11-16T19:07:55Z

app/code/core/Mage/Cms/sql/cms_setup/install-1.6.0.0.php

+            BlockInterface::DATA_STORE_ID,
+        ),
+        BlockInterface::DATA_STORE_ID,
        $installer->getTable('core/store'),
-        'store_id',
+        BlockInterface::DATA_STORE_ID,


[nitpick] Using BlockInterface::DATA_STORE_ID as the foreign key column for the core/store table is incorrect. The core/store table has its own store_id column that should be referenced. While the column names happen to be the same, semantically this should use the constant from the appropriate interface for the store table, not from BlockInterface.

Copilot · 2025-11-16T19:07:55Z

app/code/core/Mage/Cms/Model/Page.php

+    public function getTitle(): ?string
+    {
+        return $this->getDataByKey(self::DATA_TITLE);
+    }


The getTitle() method in the interface is missing an @api docblock annotation, while other similar methods in the implementation have it. Consider adding @api annotation for consistency.

Copilot · 2025-11-16T19:07:55Z

app/code/core/Mage/Cms/Model/Block.php

+    /**
+     * @api
+     */
+    public function getIdentifier(): string


The getIdentifier() method has a non-nullable string return type, but getDataByKey() could return null if the identifier is not set. This will cause a TypeError. Consider changing the return type to ?string to match the nullable behavior.

Suggested change

public function getIdentifier(): string

public function getIdentifier(): ?string

Copilot · 2025-11-16T19:07:56Z

app/code/core/Mage/Cms/Model/Page.php

+    public function getIsActive(): int
+    {
+        return (int) $this->getDataByKey(self::DATA_IS_ACTIVE);
+    }


The getIsActive() method returns a non-nullable int but doesn't cast the value before returning. The underlying data could be null or a string from the database, leading to a TypeError. Consider adding an explicit cast: return (int) $this->getDataByKey(self::DATA_IS_ACTIVE);

sonarqubecloud · 2025-11-16T19:11:02Z

Please retry analysis of this Pull-Request directly on SonarQube Cloud

update

95e9586

Copilot AI review requested due to automatic review settings November 16, 2025 19:03

sreichel added the new feature label Nov 16, 2025

github-actions bot added Component: Cms Relates to Mage_Cms Component: Adminhtml Relates to Mage_Adminhtml labels Nov 16, 2025

Merge branch 'main' into feat/interfaces

d4600b5

Copilot started reviewing on behalf of sreichel November 16, 2025 19:04 View session

Copilot finished reviewing on behalf of sreichel November 16, 2025 19:06

Copilot AI reviewed Nov 16, 2025

View reviewed changes

sreichel marked this pull request as draft November 16, 2025 19:09

	return (int) $this->getDataByKey(self::DATA_STORE_ID);
	$storeId = $this->getDataByKey(self::DATA_STORE_ID);
	return $storeId === null ? null : (int) $storeId;

		if (Mage::getStoreConfig('web/default/show_cms_breadcrumbs')
		&& ($breadcrumbs = $this->getLayout()->getBlock('breadcrumbs'))

	return $this->getDataByKey(self::DATA_STORE_ID);
	$storeId = $this->getDataByKey(self::DATA_STORE_ID);
	return $storeId === null ? null : (int) $storeId;

	public function getIdentifier(): string
	public function getIdentifier(): ?string

Uh oh!

Add Interfaces to reflect DB types #5104

Are you sure you want to change the base?

Add Interfaces to reflect DB types #5104

Uh oh!

Conversation

sreichel commented Nov 16, 2025

Description (*)

Related Pull Requests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant