From a0b0e4427829ebf215eda34956d6bb3dafe18fd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20FRANCE?= Date: Wed, 19 Aug 2026 02:18:46 +0200 Subject: [PATCH] NEW Link manage extrafields and phpunit test for the class (#39514) * NEW: Add extrafields support to Link class Link now declares isextrafieldmanaged and wires fetch_optionals()/ insertExtraFields()/deleteExtraFields() into fetch(), fetchAll(), create(), update() and delete(), matching the llx_links_extrafields table added previously. * FIX: Several bugs in Link class review - create(): wrong duplicate-record error message (copy-pasted from Societe, referenced undefined $this->name) replaced by the generic ErrorDuplicateField, consistent with update(). - create()/update(): missing "NoURL" translation key replaced by the standard ErrorFieldRequired pattern. - fetch(): guard against running an unfiltered query (no rowid, no hashforshare) that could silently return an arbitrary link; a caller in actions_linkedfiles.inc.php could hit this when 'linkid' was missing from the request. - actions_linkedfiles.inc.php: check fetch() result with `> 0` instead of a truthy test, since -1 (error) is truthy in PHP. - delete(): add User type hint (consistent with create()/update()) and a $notrigger parameter to optionally skip the LINK_DELETE trigger. - update(): fix copy-pasted docblock ("third party" -> "link"). * NEW: Add PHPUnit test for Link class Covers create/fetch/update/fetchAll/count/delete, plus regression tests for the two bugs fixed in the previous commit: create() rejects an empty url, and fetch() rejects a call with neither rowid nor hashforshare instead of returning an arbitrary record. * FIX Phan false positive on $object in actions_linkedfiles.inc.php Phan's ambient type inference for the loosely-typed global $object in this shared include file was picking up CommonSocialNetworks (an unrelated trait, not even a class), reported as undeclared ->id/->entity/->addThumbs()/->delThumbs() in a real CI Phan run on this branch, once this file was analyzed on its own via the changed-files file-list (this file has no prior baseline entry, so it was apparently never previously exercised in isolation like this). Force $object's type explicitly to CommonObject via the same @phan-var-force string-literal idiom this file already uses for $upload_dir/$upload_dirold/$confirm/$forceFullTextIndexation - CommonObject genuinely declares addThumbs()/delThumbs(), which resolves those two errors outright. $id/$entity remain reported as PhanUndeclaredProperty (CommonObject itself does not declare them, only its concrete subclasses do at runtime) - baseline-suppress that for this file the same way it is already suppressed for the sibling shared-include files actions_addupdatedelete.inc.php, actions_massactions.inc.php and actions_sendmails.inc.php, which have the exact same $object typing situation. --- dev/tools/phan/baseline.txt | 1 + htdocs/core/actions_linkedfiles.inc.php | 3 +- htdocs/core/class/link.class.php | 81 +++++-- test/phpunit/AllTests.php | 3 + test/phpunit/LinkTest.php | 267 ++++++++++++++++++++++++ 5 files changed, 335 insertions(+), 20 deletions(-) create mode 100644 test/phpunit/LinkTest.php diff --git a/dev/tools/phan/baseline.txt b/dev/tools/phan/baseline.txt index d99e3618646..7a6c6658013 100644 --- a/dev/tools/phan/baseline.txt +++ b/dev/tools/phan/baseline.txt @@ -47,6 +47,7 @@ return [ 'htdocs/compta/tva/class/paymentvat.class.php' => ['PhanUndeclaredProperty'], 'htdocs/compta/tva/clients.php' => ['PhanTypeArraySuspiciousNull', 'PhanTypeInvalidDimOffset'], 'htdocs/core/actions_addupdatedelete.inc.php' => ['PhanUndeclaredProperty'], + 'htdocs/core/actions_linkedfiles.inc.php' => ['PhanUndeclaredProperty'], 'htdocs/core/actions_massactions.inc.php' => ['PhanUndeclaredProperty'], 'htdocs/core/actions_sendmails.inc.php' => ['PhanUndeclaredProperty'], 'htdocs/core/ajax/ajaxdirtree.php' => ['PhanTypeMismatchArgument'], diff --git a/htdocs/core/actions_linkedfiles.inc.php b/htdocs/core/actions_linkedfiles.inc.php index 4e3963bf165..d265f2a280d 100644 --- a/htdocs/core/actions_linkedfiles.inc.php +++ b/htdocs/core/actions_linkedfiles.inc.php @@ -37,6 +37,7 @@ * @var string $forceFullTextIndexation */ ' +@phan-var-force CommonObject $object @phan-var-force string $upload_dir @phan-var-force string $upload_dirold @phan-var-force string $confirm @@ -224,7 +225,7 @@ if ($action == 'confirm_deletefile' && $confirm == 'yes' && !empty($permissionto $link = new Link($db); $f = $link->fetch(GETPOSTINT('linkid')); - if ($f) { + if ($f > 0) { $link->url = GETPOST('link', 'alpha'); if (substr($link->url, 0, 7) != 'http://' && substr($link->url, 0, 8) != 'https://' diff --git a/htdocs/core/class/link.class.php b/htdocs/core/class/link.class.php index 37b10cf41a2..8683ace8978 100644 --- a/htdocs/core/class/link.class.php +++ b/htdocs/core/class/link.class.php @@ -1,7 +1,7 @@ * Copyright (C) 2024-2026 MDW - * Copyright (C) 2024 Frédéric France + * Copyright (C) 2024-2026 Frédéric France * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -40,6 +40,11 @@ class Link extends CommonObject */ public $table_element = 'links'; + /** + * @var int<0,1> Does object support extrafields ? 0=No, 1=Yes + */ + public $isextrafieldmanaged = 1; + /** * @var int Entity */ @@ -102,7 +107,7 @@ class Link extends CommonObject global $langs, $conf; $error = 0; - $langs->load("errors"); + $langs->loadLangs(array("errors", "admin")); // Clean parameters if (empty($this->label)) { $this->label = trim(basename($this->url)); @@ -116,7 +121,7 @@ class Link extends CommonObject // Check parameters if (empty($this->url)) { - $this->error = $langs->trans("NoURL"); + $this->error = $langs->trans("ErrorFieldRequired", $langs->transnoentitiesnoconv("URL")); return -1; } @@ -137,12 +142,20 @@ class Link extends CommonObject $this->id = $this->db->last_insert_id($this->db->prefix()."links"); if ($this->id > 0) { - // Call trigger - $result = $this->call_trigger('LINK_CREATE', $user); + // Actions on extra fields + $result = $this->insertExtraFields(); if ($result < 0) { $error++; } - // End call triggers + + if (!$error) { + // Call trigger + $result = $this->call_trigger('LINK_CREATE', $user); + if ($result < 0) { + $error++; + } + // End call triggers + } } else { $error++; } @@ -158,7 +171,7 @@ class Link extends CommonObject } } else { if ($this->db->errno() == 'DB_ERROR_RECORD_ALREADY_EXISTS') { - $this->error = $langs->trans("ErrorCompanyNameAlreadyExists", (string) $this->name); + $this->error = $langs->trans("ErrorDuplicateField"); $result = -1; } else { $this->error = $this->db->lasterror(); @@ -170,7 +183,7 @@ class Link extends CommonObject } /** - * Update parameters of third party + * Update parameters of link * * @param User $user User executing update * @param int $call_trigger 0=no, 1=yes @@ -181,14 +194,14 @@ class Link extends CommonObject global $langs, $conf; require_once DOL_DOCUMENT_ROOT.'/core/lib/functions2.lib.php'; - $langs->load("errors"); + $langs->loadLangs(array("errors", "admin")); $error = 0; dol_syslog(get_class($this)."::Update id = ".$this->id." call_trigger = ".$call_trigger); // Check parameters if (empty($this->url)) { - $this->error = $langs->trans("NoURL"); + $this->error = $langs->trans("ErrorFieldRequired", $langs->transnoentitiesnoconv("URL")); return -1; } @@ -216,7 +229,13 @@ class Link extends CommonObject dol_syslog(get_class($this)."::update sql = ".$sql); $resql = $this->db->query($sql); if ($resql) { - if ($call_trigger) { + // Actions on extra fields + $result = $this->insertExtraFields(); + if ($result < 0) { + $error++; + } + + if (!$error && $call_trigger) { // Call trigger $result = $this->call_trigger('LINK_MODIFY', $user); if ($result < 0) { @@ -291,6 +310,10 @@ class Link extends CommonObject $link->objectid = $obj->objectid; $link->share = $obj->share; $link->share_pass = $obj->share_pass; + + // Retrieve all extrafields for link + $link->fetch_optionals(); + $links[] = $link; } return 1; @@ -345,6 +368,11 @@ class Link extends CommonObject $rowid = $this->id; } + if (empty($rowid) && empty($hashforshare)) { + $this->error = 'ErrorBadParameters'; + return -1; + } + $sqlwhere = []; $sql = "SELECT rowid, entity, datea, url, label, objecttype, objectid, share, share_pass FROM ".$this->db->prefix()."links"; @@ -377,6 +405,10 @@ class Link extends CommonObject $this->objectid = $obj->objectid; $this->share = $obj->share; $this->share_pass = $obj->share_pass; + + // Retrieve all extrafields for link + $this->fetch_optionals(); + return 1; } else { return 0; @@ -390,23 +422,26 @@ class Link extends CommonObject /** * Delete a link from database * - * @param User $user Object suer + * @param User $user Object user + * @param int<0,1> $notrigger 1=Does not execute triggers, 0=Execute triggers * @return int Return integer <0 if KO, 0 if nothing done, >0 if OK */ - public function delete($user) + public function delete(User $user, $notrigger = 0) { dol_syslog(get_class($this)."::delete", LOG_DEBUG); $error = 0; $this->db->begin(); - // Call trigger - $result = $this->call_trigger('LINK_DELETE', $user); - if ($result < 0) { - $this->db->rollback(); - return -1; + if (!$notrigger) { + // Call trigger + $result = $this->call_trigger('LINK_DELETE', $user); + if ($result < 0) { + $this->db->rollback(); + return -1; + } + // End call triggers } - // End call triggers // Remove link $sql = "DELETE FROM ".$this->db->prefix()."links"; @@ -418,6 +453,14 @@ class Link extends CommonObject $this->error = $this->db->lasterror(); } + // Removed extrafields + if (!$error) { + $result = $this->deleteExtraFields(); + if ($result < 0) { + $error++; + } + } + if (!$error) { $this->db->commit(); diff --git a/test/phpunit/AllTests.php b/test/phpunit/AllTests.php index 370d49a0c46..05a03de3ba3 100644 --- a/test/phpunit/AllTests.php +++ b/test/phpunit/AllTests.php @@ -288,6 +288,9 @@ class AllTests require_once dirname(__FILE__).'/CategorieTest.php'; $suite->addTestSuite('CategorieTest'); + require_once dirname(__FILE__).'/LinkTest.php'; + $suite->addTestSuite('LinkTest'); + require_once dirname(__FILE__).'/ProjectTest.php'; $suite->addTestSuite('ProjectTest'); require_once dirname(__FILE__).'/CommentTest.php'; diff --git a/test/phpunit/LinkTest.php b/test/phpunit/LinkTest.php new file mode 100644 index 00000000000..5ddb9327067 --- /dev/null +++ b/test/phpunit/LinkTest.php @@ -0,0 +1,267 @@ + + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * or see https://www.gnu.org/ + */ + +/** + * \file test/phpunit/LinkTest.php + * \ingroup test + * \brief PHPUnit test + * \remarks To run this script as CLI: phpunit filename.php + */ + +global $conf,$user,$langs,$db; +//define('TEST_DB_FORCE_TYPE','mysql'); // This is to force using mysql driver +//require_once 'PHPUnit/Autoload.php'; +require_once dirname(__FILE__).'/../../htdocs/master.inc.php'; +require_once dirname(__FILE__).'/../../htdocs/core/class/link.class.php'; +require_once dirname(__FILE__).'/CommonClassTest.class.php'; + +if (empty($user->id)) { + print "Load permissions for admin user nb 1\n"; + $user->fetch(1); + $user->loadRights(); +} +$conf->global->MAIN_DISABLE_ALL_MAILS = 1; + + +/** + * Class for PHPUnit tests + * + * @backupGlobals disabled + * @backupStaticAttributes enabled + * @remarks backupGlobals must be disabled to have db,conf,user and lang not erased. + */ +class LinkTest extends CommonClassTest +{ + /** + * testLinkCreate + * + * @return int + */ + public function testLinkCreate() + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $localobject = new Link($db); + $localobject->url = 'https://www.dolibarr.org'; + $localobject->label = 'Specimen link'; + $localobject->objecttype = 'societe'; + $localobject->objectid = 1; + + $result = $localobject->create($user); + print __METHOD__." result=".$result."\n"; + $this->assertGreaterThan(0, $result); + + return $result; + } + + /** + * testLinkCreateWithoutUrl + * + * A link with no url must be rejected. + * + * @return void + * + * @depends testLinkCreate + * The depends says test is run only if previous is ok + */ + public function testLinkCreateWithoutUrl() + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $localobject = new Link($db); + $localobject->objecttype = 'societe'; + $localobject->objectid = 1; + + $result = $localobject->create($user); + print __METHOD__." result=".$result."\n"; + $this->assertLessThan(0, $result); + } + + /** + * testLinkFetch + * + * @param int $id Id of link + * @return Link + * + * @depends testLinkCreate + * The depends says test is run only if previous is ok + */ + public function testLinkFetch($id) + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $localobject = new Link($db); + $result = $localobject->fetch($id); + + print __METHOD__." id=".$id." result=".$result."\n"; + $this->assertGreaterThan(0, $result); + $this->assertEquals('https://www.dolibarr.org', $localobject->url); + $this->assertEquals('Specimen link', $localobject->label); + + return $localobject; + } + + /** + * testLinkFetchWithoutParameters + * + * A fetch() with neither rowid nor hashforshare must fail instead of + * returning an arbitrary record. + * + * @return void + * + * @depends testLinkCreate + * The depends says test is run only if previous is ok + */ + public function testLinkFetchWithoutParameters() + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $localobject = new Link($db); + $result = $localobject->fetch(0); + + print __METHOD__." result=".$result."\n"; + $this->assertLessThan(0, $result); + } + + /** + * testLinkUpdate + * + * @param Link $localobject Link + * @return Link + * + * @depends testLinkFetch + * The depends says test is run only if previous is ok + */ + public function testLinkUpdate($localobject) + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $localobject->label = 'Specimen link updated'; + $result = $localobject->update($user); + + print __METHOD__." id=".$localobject->id." result=".$result."\n"; + $this->assertGreaterThan(0, $result); + + $localobject->fetch($localobject->id); + $this->assertEquals('Specimen link updated', $localobject->label); + + return $localobject; + } + + /** + * testLinkFetchAll + * + * @param Link $localobject Link + * @return Link + * + * @depends testLinkUpdate + * The depends says test is run only if previous is ok + */ + public function testLinkFetchAll($localobject) + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $links = array(); + $tmpobject = new Link($db); + $result = $tmpobject->fetchAll($links, $localobject->objecttype, $localobject->objectid); + + print __METHOD__." result=".$result."\n"; + $this->assertGreaterThan(0, $result); + $this->assertGreaterThan(0, count($links)); + + return $localobject; + } + + /** + * testLinkCount + * + * @param Link $localobject Link + * @return Link + * + * @depends testLinkFetchAll + * The depends says test is run only if previous is ok + */ + public function testLinkCount($localobject) + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $nb = Link::count($db, $localobject->objecttype, $localobject->objectid); + + print __METHOD__." nb=".$nb."\n"; + $this->assertGreaterThan(0, $nb); + + return $localobject; + } + + /** + * testLinkDelete + * + * @param Link $localobject Link + * @return int + * + * @depends testLinkCount + * The depends says test is run only if previous is ok + */ + public function testLinkDelete($localobject) + { + global $conf,$user,$langs,$db; + $conf = $this->savconf; + $user = $this->savuser; + $langs = $this->savlangs; + $db = $this->savdb; + + $result = $localobject->delete($user); + + print __METHOD__." id=".$localobject->id." result=".$result."\n"; + $this->assertGreaterThan(0, $result); + + $resultFetch = $localobject->fetch($localobject->id); + print __METHOD__." resultFetch=".$resultFetch."\n"; + $this->assertEquals(0, $resultFetch); + + return $result; + } +}