Skip to content

refactor(psalm): Fix most issues with the workflowengine #55140

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.

Sign up for GitHub

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

Merged
nickvergessen merged 1 commit into from
Sep 22, 2025
Merged

refactor(psalm): Fix most issues with the workflowengine #55140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions apps/workflowengine/lib/BackgroundJobs/Rotate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use OCA\WorkflowEngine\AppInfo\Application;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\TimedJob;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\Log\RotationTrait;
use OCP\Server;
Expand All @@ -21,17 +22,18 @@ public function __construct(ITimeFactory $time) {
$this->setInterval(60 * 60 * 3);
}

protected function run($argument) {
protected function run($argument): void {
$config = Server::get(IConfig::class);
$default = $config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/flow.log';
$this->filePath = trim((string)$config->getAppValue(Application::APP_ID, 'logfile', $default));
$appConfig = Server::get(IAppConfig::class);
$default = $config->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data') . '/flow.log';
$this->filePath = trim($appConfig->getValueString(Application::APP_ID, 'logfile', $default));

if ($this->filePath === '') {
// disabled, nothing to do
return;
}

$this->maxSize = $config->getSystemValue('log_rotate_size', 100 * 1024 * 1024);
$this->maxSize = $config->getSystemValueInt('log_rotate_size', 100 * 1024 * 1024);

if ($this->shouldRotateBySize()) {
$this->rotate();
Expand Down
15 changes: 5 additions & 10 deletions apps/workflowengine/lib/Check/AbstractStringCheck.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@

abstract class AbstractStringCheck implements ICheck {

/** @var array[] Nested array: [Pattern => [ActualValue => Regex Result]] */
protected $matches;
/** @var array<string, array<string, false|int>> $matches Nested array: [Pattern => [ActualValue => Regex Result]] */
protected array $matches;

/**
* @param IL10N $l
Expand Down Expand Up @@ -64,13 +64,13 @@ protected function executeStringCheck($operator, $checkValue, $actualValue) {
* @param string $value
* @throws \UnexpectedValueException
*/
public function validateCheck($operator, $value) {
public function validateCheck($operator, $value): void {
if (!in_array($operator, ['is', '!is', 'matches', '!matches'])) {
throw new \UnexpectedValueException($this->l->t('The given operator is invalid'), 1);
}

if (in_array($operator, ['matches', '!matches'])
&& @preg_match($value, null) === false) {
&& @preg_match($value, '') === false) {
throw new \UnexpectedValueException($this->l->t('The given regular expression is invalid'), 2);
}
}
Expand All @@ -85,12 +85,7 @@ public function isAvailableForScope(int $scope): bool {
return $scope === IManager::SCOPE_ADMIN;
}

/**
* @param string $pattern
* @param string $subject
* @return int|bool
*/
protected function match($pattern, $subject) {
protected function match(string $pattern, string $subject): int|false {
$patternHash = md5($pattern);
$subjectHash = md5($subject);
if (isset($this->matches[$patternHash][$subjectHash])) {
Expand Down
48 changes: 18 additions & 30 deletions apps/workflowengine/lib/Check/FileSize.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,49 +14,40 @@

class FileSize implements ICheck {

/** @var int */
protected $size;
protected int|float|null $size = null;

/**
* @param IL10N $l
* @param IRequest $request
*/
public function __construct(
protected IL10N $l,
protected IRequest $request,
protected readonly IL10N $l,
protected readonly IRequest $request,
) {
}

/**
* @param string $operator
* @param string $value
* @return bool
*/
public function executeCheck($operator, $value) {
public function executeCheck($operator, $value): bool {
$size = $this->getFileSizeFromHeader();
if ($size === false) {
return false;
}

$value = Util::computerFileSize($value);
if ($size !== false) {
switch ($operator) {
case 'less':
return $size < $value;
case '!less':
return $size >= $value;
case 'greater':
return $size > $value;
case '!greater':
return $size <= $value;
}
}
return false;
return match ($operator) {
'less' => $size < $value,
'!less' => $size >= $value,
'greater' => $size > $value,
'!greater' => $size <= $value,
default => false,
};
}

/**
* @param string $operator
* @param string $value
* @throws \UnexpectedValueException
*/
public function validateCheck($operator, $value) {
public function validateCheck($operator, $value): void {
if (!in_array($operator, ['less', '!less', 'greater', '!greater'])) {
throw new \UnexpectedValueException($this->l->t('The given operator is invalid'), 1);
}
Expand All @@ -66,10 +57,7 @@ public function validateCheck($operator, $value) {
}
}

/**
* @return string
*/
protected function getFileSizeFromHeader() {
protected function getFileSizeFromHeader(): int|float|false {
if ($this->size !== null) {
return $this->size;
}
Expand All @@ -81,11 +69,11 @@ protected function getFileSizeFromHeader() {
}
}

if ($size === '') {
if ($size === '' || !is_numeric($size)) {
$size = false;
}

$this->size = $size;
$this->size = Util::numericToNumber($size);
return $this->size;
}

Expand Down
24 changes: 7 additions & 17 deletions apps/workflowengine/lib/Check/RequestRemoteAddress.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ public function executeCheck($operator, $value) {
$decodedValue = explode('/', $value);

if ($operator === 'matchesIPv4') {
return $this->matchIPv4($actualValue, $decodedValue[0], $decodedValue[1]);
return $this->matchIPv4($actualValue, $decodedValue[0], (int)$decodedValue[1]);
} elseif ($operator === '!matchesIPv4') {
return !$this->matchIPv4($actualValue, $decodedValue[0], $decodedValue[1]);
return !$this->matchIPv4($actualValue, $decodedValue[0], (int)$decodedValue[1]);
} elseif ($operator === 'matchesIPv6') {
return $this->matchIPv6($actualValue, $decodedValue[0], $decodedValue[1]);
return $this->matchIPv6($actualValue, $decodedValue[0], (int)$decodedValue[1]);
} else {
return !$this->matchIPv6($actualValue, $decodedValue[0], $decodedValue[1]);
return !$this->matchIPv6($actualValue, $decodedValue[0], (int)$decodedValue[1]);
}
}

Expand Down Expand Up @@ -76,12 +76,8 @@ public function validateCheck($operator, $value) {

/**
* Based on https://stackoverflow.com/a/594134
* @param string $ip
* @param string $rangeIp
* @param int $bits
* @return bool
*/
protected function matchIPv4($ip, $rangeIp, $bits) {
protected function matchIPv4(string $ip, string $rangeIp, int $bits): bool {
$rangeDecimal = ip2long($rangeIp);
$ipDecimal = ip2long($ip);
$mask = -1 << (32 - $bits);
Expand All @@ -90,12 +86,8 @@ protected function matchIPv4($ip, $rangeIp, $bits) {

/**
* Based on https://stackoverflow.com/a/7951507
* @param string $ip
* @param string $rangeIp
* @param int $bits
* @return bool
*/
protected function matchIPv6($ip, $rangeIp, $bits) {
protected function matchIPv6(string $ip, string $rangeIp, int $bits): bool {
$ipNet = inet_pton($ip);
$binaryIp = $this->ipv6ToBits($ipNet);
$ipNetBits = substr($binaryIp, 0, $bits);
Expand All @@ -109,10 +101,8 @@ protected function matchIPv6($ip, $rangeIp, $bits) {

/**
* Based on https://stackoverflow.com/a/7951507
* @param string $packedIp
* @return string
*/
protected function ipv6ToBits($packedIp) {
protected function ipv6ToBits(string $packedIp): string {
$unpackedIp = unpack('A16', $packedIp);
$unpackedIp = str_split($unpackedIp[1]);
$binaryIp = '';
Expand Down
4 changes: 1 addition & 3 deletions apps/workflowengine/lib/Check/TFileCheck.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/
namespace OCA\WorkflowEngine\Check;

use OCA\WorkflowEngine\AppInfo\Application;
use OCA\WorkflowEngine\Entity\File;
use OCP\Files\Node;
use OCP\Files\NotFoundException;
Expand Down Expand Up @@ -44,8 +43,7 @@ public function setEntitySubject(IEntity $entity, $subject): void {
if ($entity instanceof File) {
if (!$subject instanceof Node) {
throw new \UnexpectedValueException(
'Expected Node subject for File entity, got {class}',
['app' => Application::APP_ID, 'class' => get_class($subject)]
Copy link
Member

@nickvergessen nickvergessen Sep 17, 2025

Choose a reason for hiding this comment

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

This looks like it should be a log message instead of an exception :D

Copy link
Member Author

@CarlSchwan CarlSchwan Sep 17, 2025

Choose a reason for hiding this comment

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

I think being an exception here is correct as giving anything else than a Node is a fatal logic error, there was just a mixup on the correct way to creates the error message.

'Expected Node subject for File entity, got ' . get_class($subject),
);
}
$this->storage = $subject->getStorage();
Expand Down
3 changes: 1 addition & 2 deletions apps/workflowengine/lib/Controller/GlobalWorkflowsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@

class GlobalWorkflowsController extends AWorkflowController {

/** @var ScopeContext */
private $scopeContext;
private ?ScopeContext $scopeContext = null;

protected function getScopeContext(): ScopeContext {
if ($this->scopeContext === null) {
Expand Down
25 changes: 8 additions & 17 deletions apps/workflowengine/lib/Entity/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\SystemTag\ISystemTag;
use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\MapperEvent;
use OCP\WorkflowEngine\EntityContext\IContextPortation;
Expand All @@ -34,16 +33,10 @@

class File implements IEntity, IDisplayText, IUrl, IIcon, IContextPortation {
private const EVENT_NAMESPACE = '\OCP\Files::';
/** @var string */
protected $eventName;
/** @var Event */
protected $event;
/** @var ?Node */
private $node;
/** @var ?IUser */
private $actingUser = null;
/** @var UserMountCache */
private $userMountCache;
protected ?string $eventName = null;
protected ?Event $event = null;
private ?Node $node = null;
private ?IUser $actingUser = null;

public function __construct(
protected IL10N $l10n,
Expand All @@ -52,10 +45,9 @@ public function __construct(
private IUserSession $userSession,
private ISystemTagManager $tagManager,
private IUserManager $userManager,
UserMountCache $userMountCache,
private UserMountCache $userMountCache,
private IMountManager $mountManager,
) {
$this->userMountCache = $userMountCache;
}

public function getName(): string {
Expand Down Expand Up @@ -185,7 +177,6 @@ public function getDisplayText(int $verbosity = 0): string {
$tagIDs = $this->event->getTags();
$tagObjects = $this->tagManager->getTagsByIds($tagIDs);
foreach ($tagObjects as $systemTag) {
/** @var ISystemTag $systemTag */
if ($systemTag->isUserVisible()) {
$tagNames[] = $systemTag->getName();
}
Expand All @@ -198,15 +189,15 @@ public function getDisplayText(int $verbosity = 0): string {
}
array_push($options, $tagString, $filename);
return $this->l10n->t('%s assigned %s to %s', $options);
default:
return '';
}
}

public function getUrl(): string {
try {
return $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $this->getNode()->getId()]);
} catch (InvalidPathException $e) {
return '';
} catch (NotFoundException $e) {
} catch (InvalidPathException|NotFoundException) {
return '';
}
}
Expand Down
12 changes: 10 additions & 2 deletions apps/workflowengine/lib/Helper/LogContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,16 @@
use OCP\WorkflowEngine\IOperation;

class LogContext {
/** @var array */
protected $details;
/** @var array{
* message?: string,
* scopes?: array,
* operation?: array{class: class-string<IOperation>, name: string},
* entity?: array{class: class-string<IEntity>, name: string},
* configuration?: array,
* eventName?: string,
* }
*/
protected array $details;

public function setDescription(string $description): LogContext {
$this->details['message'] = $description;
Expand Down
15 changes: 3 additions & 12 deletions apps/workflowengine/lib/Helper/ScopeContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,9 @@
use OCP\WorkflowEngine\IManager;

class ScopeContext {
/** @var int */
private $scope;
/** @var string */
private $scopeId;
/** @var string */
private $hash;
private int $scope;
private string $scopeId;
private ?string $hash = null;

public function __construct(int $scope, ?string $scopeId = null) {
$this->scope = $this->evaluateScope($scope);
Expand All @@ -38,16 +35,10 @@ private function evaluateScopeId(?string $scopeId = null): string {
return trim((string)$scopeId);
}

/**
* @return int
*/
public function getScope(): int {
return $this->scope;
}

/**
* @return string
*/
public function getScopeId(): string {
return $this->scopeId;
}
Expand Down
Loading
Loading