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

API Move logic from silverstripe/cms into central place #11460

Merged
merged 1 commit into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions src/Forms/GridField/GridFieldDataColumns.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,12 @@
use InvalidArgumentException;
use LogicException;
use SilverStripe\Model\ModelData;
use SilverStripe\Dev\Deprecation;
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated but since it's not used anymore I removed it


/**
* @see GridField
*/
class GridFieldDataColumns extends AbstractGridFieldComponent implements GridField_ColumnProvider
{

/**
* @var array
*/
Expand All @@ -31,6 +29,15 @@ class GridFieldDataColumns extends AbstractGridFieldComponent implements GridFie
*/
protected $displayFields = [];

private bool $displayStatusFlags = true;

private array $columnsForStatusFlag = [
'Title',
'Name',
];
Comment on lines +34 to +37
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 20, 2024

Choose a reason for hiding this comment

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

These were the defaults in VersionedGridFieldState (see https://github.com/silverstripe/silverstripe-versioned/pull/479/files#diff-c3dbb0835d959d49ae741afb170b84162d26849c69dce38c55530e9338980b46)

All of the new status flag code in this class is adapted from there.


private ?string $statusFlagColumn = null;

/**
* Modify the list of columns displayed in the table.
* See {@link GridFieldDataColumns->getDisplayFields()} and {@link GridFieldDataColumns}.
Expand All @@ -44,6 +51,10 @@ public function augmentColumns($gridField, &$columns)

foreach ($baseColumns as $col) {
$columns[] = $col;
// Find the column to add status flags to
if ($this->statusFlagColumn === null && in_array($col, $this->getColumnsForStatusFlag())) {
$this->statusFlagColumn = $col;
}
}

$columns = array_unique($columns ?? []);
Expand All @@ -60,6 +71,45 @@ public function getColumnsHandled($gridField)
return array_keys($this->getDisplayFields($gridField) ?? []);
}

/**
* Set whether status flags are displayed in this gridfield
*/
public function setDisplayStatusFlags(bool $display): static
{
$this->displayStatusFlags = $display;
return $this;
}

/**
* Get whether status flags are displayed in this gridfield
*/
public function getDisplayStatusFlags(): bool
{
return $this->displayStatusFlags;
}

/**
* Set which columns can be used to display the status flags.
* The first column from this list found in the gridfield will be used.
*/
public function setColumnsForStatusFlag(array $columns): static
{
if (empty($columns)) {
throw new InvalidArgumentException('Columns array must not be empty');
}
$this->columnsForStatusFlag = $columns;
return $this;
}

/**
* Get which columns can be used to display the status flags.
* The first column from this list found in the gridfield will be used.
*/
public function getColumnsForStatusFlag(): array
{
return $this->columnsForStatusFlag;
}

/**
* Override the default behaviour of showing the models summaryFields with
* these fields instead
Expand Down Expand Up @@ -183,6 +233,11 @@ public function getColumnContent($gridField, $record, $columnName)
// Do any final escaping
$value = $this->escapeValue($gridField, $value);

// Add on status flags
if ($this->getDisplayStatusFlags() && $columnName === $this->statusFlagColumn) {
$value .= $record->getStatusFlagMarkup('ss-gridfield-badge');
}

return $value;
}

Expand Down
12 changes: 10 additions & 2 deletions src/Forms/GridField/GridFieldDetailForm_ItemRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use SilverStripe\View\HTML;
use SilverStripe\View\SSViewer;
use SilverStripe\Model\ModelData;
use SilverStripe\ORM\FieldType\DBField;

class GridFieldDetailForm_ItemRequest extends RequestHandler
{
Expand Down Expand Up @@ -930,11 +931,13 @@ public function Breadcrumbs($unlinked = false)
$items = $this->popupController->Breadcrumbs($unlinked);

if (!$items) {
/** @var ArrayList<ArrayData> $items */
$items = ArrayList::create();
}

if ($this->record && $this->record->ID) {
$title = ($this->record->Title) ? $this->record->Title : "#{$this->record->ID}";
$record = $this->getRecord();
if ($record && $record->ID) {
$title = ($record->Title) ? $record->Title : "#{$record->ID}";
Comment on lines -936 to +940
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactoring slightly so I can use the $record variable lower down - also updating this to use best practice (public getter better than protected property)

$items->push(ArrayData::create([
'Title' => $title,
'Link' => $this->Link()
Expand All @@ -952,6 +955,11 @@ public function Breadcrumbs($unlinked = false)
}
}

$statusFlags = $record->getStatusFlagMarkup('badge--breadcrumbs');
if ($statusFlags) {
$items->last()->setField('Extra', DBField::create_field('HTMLFragment', $statusFlags));
}

$this->extend('updateBreadcrumbs', $items);
return $items;
}
Expand Down
61 changes: 61 additions & 0 deletions src/Model/ModelData.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\Model\ArrayData;
use SilverStripe\View\CastingService;
use SilverStripe\View\HTML;
use SilverStripe\View\SSViewer;
use UnexpectedValueException;

Expand Down Expand Up @@ -47,6 +48,7 @@ class ModelData
private static array $casting = [
'CSSClasses' => 'Varchar',
'forTemplate' => 'HTMLText',
'StatusFlagMarkup' => 'HTMLFragment',
];

/**
Expand Down Expand Up @@ -74,6 +76,8 @@ class ModelData

private array $objCache = [];

private $_cache_statusFlags = null;

public function __construct()
{
// no-op
Expand Down Expand Up @@ -487,6 +491,52 @@ public function hasValue(string $field, array $arguments = [], bool $cache = tru

// UTILITY METHODS -------------------------------------------------------------------------------------------------

/**
* Flags provides the user with additional data about the current page status.
*
* Mostly this is used for versioning, but can be used for other purposes (e.g. localisation).
* Each page can have more than one status flag.
*
* Returns an associative array of a unique key to a (localized) title for the flag.
* The unique key can be reused as a CSS class.
*
* Example (simple):
* "deletedonlive" => "Deleted"
*
* Example (with optional title attribute):
* "deletedonlive" => ['text' => "Deleted", 'title' => 'This page has been deleted']
*/
public function getStatusFlags(bool $cached = true): array
Copy link
Member Author

Choose a reason for hiding this comment

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

This method could be useful for any type of model. We currently do this in a couple of different ways so this gives us a standard method we can start using across the board (in gridfield, in CMSMain, in linkfield, etc etc).

Note that actually starting to use it in all those places is out of scope for this set of PRs. For now I'm just putting it in a standard place so the generic version of CMSMain has a method to call that it knows will always be available.

Versioned is currently the only thing I know for sure will be using it consistently (see the related versioned PR) though it uses the same extension hook this method used on SiteTree so other modules could already be hooking into it.

Copy link
Member

@emteknetnz emteknetnz Nov 14, 2024

Choose a reason for hiding this comment

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

This seems ... kind of out of scope?

I don't like the idea of adding generic functionality that we might use in the future, but we're not actually planning on implementing straight away

Also the method name for this is wrong for a generic method as 'status flags' is clearly something for versioning

It feels like this method should just live on the Versioned extension instead of on ModelData?

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 14, 2024

Choose a reason for hiding this comment

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

This seems ... kind of out of scope?

The method is on SiteTree right now. It gets called from CMSMain, so it needs to be available on any DataObject with the Hierarchy extension.

I thought of moving it to DataObject but decided to put it here instead because it could be useful for models that don't live in the database as well. If I put it on DataObject it'd likely end up being moved here anyway when we implement #11385 ("Move logic out of DataObject that isn't needed for ORM interactions.")

I don't like the idea of adding generic functionality that we might use in the future, but we're not actually planning on implementing straight away

It's not being added, just moved. It's already on SiteTree. This just makes it available on other models as well.
It does get used right away for other versioned or fluent data objects. We're not likely to ever use it in core on arbitrary ModelData classes but this allows projects to do so.

Also the method name for this is wrong for a generic method as 'status flags' is clearly something for versioning

This isn't just for versioning, e.g. fluent uses it: https://github.com/tractorcow-farm/silverstripe-fluent/blob/208b365b66005ce263dbc7ae402fe33885b26d3d/src/Extension/FluentFilteredExtension.php#L61

The method name is also not changing because this is just moving the method not adding a new one. Renaming the methods from SiteTree is not the intention here, we're just moving them to somewhere CMSMain can rely on them existing.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of that said:
It either has to be here or on DataObject.

I can move it to DataObject if you prefer just to get us moving along, but since it's not got anything to do with the ORM it doesn't really belong there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like the idea of adding generic functionality that we might use in the future, but we're not actually planning on implementing straight away

If it makes you happier about this, I'd be okay with widening the scope of the change in silverstripe/silverstripe-versioned#479 to move the call to statusFlags() into framework, so all gridfields use it right away.

Copy link
Member

Choose a reason for hiding this comment

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

Does the fluent example currently only apply the status flags on SiteTree and not other DataObjects

If you did all of the above, would it result in some sort of UX change for regular DataObjects in gridfields? Or is it just adding an essentially unused cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the fluent example currently only apply the status flags on SiteTree and not other DataObjects

Without this PR, yes, because this method only exists on SiteTree right now. This PR will automatically make it apply to all DataObject models which have the fluent extension applied, since it's the same extension hook - but the flags won't be displayed outside of a CMSMain unless the model is also versioned.

If you did all of the above, would it result in some sort of UX change for regular DataObjects in gridfields? Or is it just adding an essentially unused cache?

I'm not sure what "all of the above" is... so to cover my bases:

  • if this PR is merged, any module or project can apply any status flag to any ModelData class - but they'll only be displayed in a CMSMain or if the model is versioned.
  • if I widen the change in API Update API to reflect changes in silverstripe/framework silverstripe-versioned#479 all models will have status flags displayed in gridfields. Right now only versioned models will have their flags displayed in gridfields because it's a versioned extension that adds that display.

Copy link
Member

Choose a reason for hiding this comment

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

OK sounds good, so if we widen it would it "just work" from a UX point of view? i.e. would the status flag icons now show on arbitary versioned DataObjects in a GridField with little need for us to modify things?

Copy link
Member Author

Choose a reason for hiding this comment

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

To check that I'll have to implement the change, and at that point we may as well just include that in the PRs. Do you want me to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah go for it

{
if (!$this->_cache_statusFlags || !$cached) {
$flags = [];
$this->extend('updateStatusFlags', $flags);
$this->_cache_statusFlags = $flags;
}
return $this->_cache_statusFlags;
}

/**
* Get the HTML markup for rendering status flags for this model.
*/
public function getStatusFlagMarkup(string $additionalCssClass = ''): string
{
$flagContent = '';
foreach ($this->getStatusFlags() as $class => $data) {
$flagAttributes = [
'class' => rtrim("badge status-{$class} $additionalCssClass"),
];
if (is_string($data)) {
$data = ['text' => $data];
}
if (isset($data['title'])) {
$flagAttributes['title'] = $data['title'];
}
$flagContent .= HTML::createTag('span', $flagAttributes, Convert::raw2xml($data['text']));
}
return $flagContent;
}

/**
* Find appropriate templates for SSViewer to use to render this object
*/
Expand Down Expand Up @@ -545,6 +595,17 @@ public function Debug(): ModelData|string
return ModelDataDebugger::create($this);
}

/**
* Clears record-specific cached data.
*/
public function flushCache(): static
Copy link
Member Author

Choose a reason for hiding this comment

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

Same method that used to clear this static cache in SiteTree - this means people can just keep calling the same method if they need these flags cleared.

{
$this->objCacheClear();
$this->_cache_statusFlags = null;
$this->extend('onFlushCache');
return $this;
}

/**
* Generate the cache name for a field
*/
Expand Down
50 changes: 13 additions & 37 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,17 +116,13 @@ class DataObject extends ModelData implements DataObjectInterface, i18nEntityPro
{
/**
* Human-readable singular name.
* @var string
* @config
*/
private static $singular_name = null;
private static ?string $singular_name = null;

/**
* Human-readable plural name
* @var string
* @config
*/
private static $plural_name = null;
private static ?string $plural_name = null;
Comment on lines -119 to +125
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated tidy-up and strong typing for these config


/**
* Description of the class.
Expand All @@ -150,7 +146,6 @@ class DataObject extends ModelData implements DataObjectInterface, i18nEntityPro
* @var string
*/
private static $default_classname = null;

/**
* Whether this DataObject class must only use the primary database and not a read-only replica
* Note that this will be only be enforced when using DataQuery::execute() or
Expand Down Expand Up @@ -957,40 +952,23 @@ public function i18n_plural_name()

/**
* Get description for this class
* @return null|string
*/
public function classDescription()
public function classDescription(): ?string
{
return static::config()->get('class_description', Config::UNINHERITED);
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally uninherited because the two classes that use this right now (SiteTree and BaseElement) both want the config to be uninherited.

Makes sense - the subclass should have a more specific description so it's clear how it's different from the superclass.

}

/**
* Get localised description for this class
* @return null|string
*/
public function i18n_classDescription()
public function i18n_classDescription(): ?string
Copy link
Member Author

Choose a reason for hiding this comment

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

I know new method names shouldn't have underscore, but I've kept this one in for two reasons:

  1. This method is being moved from SiteTree::i18n_classDescription() - keeping the same name reduces upgrade pain.
  2. This method name is consistent with the other i18n_* methods on this class.

{
$notDefined = 'NOT_DEFINED';
$baseDescription = $this->classDescription() ?? $notDefined;

// Check the new i18n key first
$description = _t(static::class . '.CLASS_DESCRIPTION', $baseDescription);
if ($description !== $baseDescription) {
return $description;
}

// Fall back on the deprecated localisation key
$legacyI18n = _t(static::class . '.DESCRIPTION', $baseDescription);
if ($legacyI18n !== $baseDescription) {
return $legacyI18n;
}

// If there was no description available in config nor in i18n, return null
if ($baseDescription === $notDefined) {
$description = _t(static::class.'.CLASS_DESCRIPTION', $this->classDescription() ?? $notDefined);
if ($description === $notDefined) {
return null;
}
// Return raw description
return $baseDescription;
return $description;
}

/**
Expand Down Expand Up @@ -3562,14 +3540,14 @@ public static function get_one($callerClass = null, $filter = "", $cache = true,
}

/**
* Flush the cached results for all relations (has_one, has_many, many_many)
* Also clears any cached aggregate data.
* @inheritDoc
*
* Also flush the cached results for all relations (has_one, has_many, many_many)
*
* @param boolean $persistent When true will also clear persistent data stored in the Cache system.
* @param bool $persistent When true will also clear persistent data stored in the Cache system.
* When false will just clear session-local cached data
* @return static $this
*/
public function flushCache($persistent = true)
public function flushCache(bool $persistent = true): static
Comment on lines +3545 to +3550
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding a little more description and hardcoding while I'm here. See ModelData::flushCache().

{
if (static::class == DataObject::class) {
DataObject::$_cache_get_one = [];
Expand All @@ -3583,11 +3561,9 @@ public function flushCache($persistent = true)
}
}

$this->extend('onFlushCache');

$this->components = [];
$this->eagerLoadedData = [];
return $this;
return parent::flushCache();
}

/**
Expand Down
11 changes: 0 additions & 11 deletions src/ORM/FieldType/DBEnum.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Validation\FieldValidation\OptionFieldValidator;
use SilverStripe\Core\Resettable;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Forms\DropdownField;
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\SelectField;
Expand Down Expand Up @@ -43,16 +42,6 @@ class DBEnum extends DBString implements Resettable
*/
protected static array $enum_cache = [];

/**
* Clear all cached enum values.
* @deprecated 5.4.0 Use reset() instead.
*/
public static function flushCache(): void
{
Deprecation::notice('5.4.0', 'Use reset() instead.');
static::reset();
}

public static function reset(): void
{
DBEnum::$enum_cache = [];
Expand Down
Loading
Loading