* FIX Drag and drop of a file reports a wrong error, or none at all
The error handler of dragAndDropFileUpload() had three defects that all end on
the user believing the file was attached when it was not.
- The first assignment of the message was dead code, there was no return after
the test on the http code 403, so a refusal was reported as a generic error.
- The key ErrorUploadPermissionDenied it used exists in no language file, so
the raw key was shown.
- JSON.parse() was called with no try/catch on an answer that is not always a
json, a fatal error of the endpoint or a request over post_max_size for
example. The exception left the user on a page with no message at all.
- An empty list of files was treated as a success, while it means the endpoint
stored nothing.
The value of PHP_SELF is also escaped before it is written into the 6 generated
javascript strings. It holds the path info of the request on a server that
accepts it, so it is a user input. dol_escape_js() is called with the mode that
escapes a double quote by a double quote, the strings being delimited by double
quotes, and the sequence '</' is escaped too because the function does not do it
and the path info could otherwise close the script tag and open one of its own.
Adds the key ErrorOnAtLeastOneFileUpload to en_US, used when some files of a
batch failed and some did not.
* FIX getMultidirOutput returns a directory the Documents tab does not read
Four defects of the same function, all ending on a file stored where the user
will never see it, or written outside the documents directory.
- The ref of the project of a task was only passed through dol_sanitizePathName(),
which keeps a slash, a colon and the accented chars, while projet/tasks/document.php
sanitizes it with dol_sanitizeFileName(). A project ref holding a slash even
created an extra level of directory. Measured on real databases: 304 projects
over 330 hold a slash on one of them, carrying 701 tasks over 824.
- The same case calls $object->fetchProject() with no guard, while the signature
of the function accepts an object that is not a CommonObject, and even a null
when a module is given. Such a caller gets a fatal error where it expects the
error string. No core caller is in that case today, an external module or a
hook can be.
- The entity of the object may have no declared directory, an object shared by
another entity for example. The undefined index returned a relative path, so
the caller read or wrote under the web root. The current entity is used
instead, and the fallback is logged because the directory is then not the one
of the entity of the object, which matters for a caller that deletes files.
- When the current entity has no declared directory either, the fallback
returned the sub directory alone, again a relative path. The same error than
for a module that declares no directory at all is now returned.
Adds the sub directory of a partnership and of a stock transfer, which their own
document tabs already read.
* FIX getElementProperties answers wrong properties for 9 elements
A customer payment, a supplier payment, a various payment, a stock transfer and
the 4 objects of the hrm module had no properties at all, or wrong ones, so any
caller that resolves a class, a table or a document directory from an element
failed on them.
- payment, payment_supplier and payment_various had no branch. The branch of a
customer payment tests $elementType and not $element, because the rule on the
elements named myobject_mysubobject rewrites $element to 'payment' for
'payment_salary' too, which is stored somewhere else.
- job, position, skill and evaluation answered a wrong table (hrm_job_user for
a position, and so on) and no sub directory, while their document tabs read
one named after the element.
- stocktransfer answered an empty classname, because it is not the ucfirst() of
the element, so a caller doing new $classname($db) ended on a fatal error.
- The sub directory was concatenated even when the module is disabled and the
directory is empty, which answered a path at the root of the file system.
- A contact and a conference are stored into a sub directory their tab reads.
isModEnabled('invoice') is tested for a customer payment: there is no module
named 'compta', so testing it was always false, while $conf->compta->payment is
set unconditionally by Conf::setValues() and could not be used as a proxy.
* FIX Access refused to everyone on 11 objects of the core
restrictedArea() and checkUserAccessToObject() refuse the access to objects that
no permission and no rule can match, whatever the user, an administrator
included.
- The hrm module declares no permission at its first level, only 'all', and the
stocktransfer module only 'stocktransfer'. A check on the module itself
therefore tests a permission that does not exist. The mapping is the same one
as into User::hasRight().
- The module of an event organization declares no permission at all, its whole
permission block being commented out on purpose, and its cards check the
parent project instead. The feature is mapped onto that project, with the two
guards the card has: an external user is refused, and so is a conference with
no parent project, whose id of 0 would otherwise grant an access with no check
on the record at all.
- The default rule of checkUserAccessToObject() builds its sql on the columns
entity and fk_soc of the table. llx_asset, llx_paiement, llx_paiementfourn and
llx_workstation_workstation have no fk_soc, and llx_hrm_job, llx_hrm_job_user
and llx_hrm_skill have neither. The sql failed, so the access was refused to
every user this rule applies to. These tables are now checked on their entity
only, which is what the $check rule already does for the same class of tables,
and the 3 tables of hrm can be checked on nothing at all. The rule is selected
on the table and not on the element, because $object is an id and not an
object for most of the callers, the cards of an asset and of a workstation
included, which are broken today for any user without the permission to see
all third parties.
- An external user is refused explicitly on those tables: none of these objects
is linked to a third party, so the default rule refused him through a link
that does not exist, and the rules that replace it do not look at the third
party of the user at all.
Measured on a vanilla instance with 5 profiles, an administrator, an internal
user with every right, one without the permission to see all third parties, one
that is not a sales representative of the third party of the object, and an
external user: the 11 objects go from refused to granted for the internal users
and stay refused for the external one, and the 26 other elements answer exactly
the same for the 5 profiles.
* FIX A file dropped on a card is lost, or reported as refused when it was stored
FileUpload stores the file into a directory that the "Attached files" tab of the
object never reads, so the user attaches a file that no screen will ever show,
and nothing is indexed in database to find it back. Measured on real databases:
216487 thirdparties over 216887 and 157852 products over 280319 are in that case
on the cards that already enable the drag and drop.
- The directory of the object is now forged with get_exdir(), the way the tabs
do: it always uses the id for a thirdparty, whose ref is a company name and is
not unique, and it falls back on the id when the ref is empty. The sub
directory of the module is read with getMultidirOutput(), which knows the
elements that store their documents into one. That function does not return
an empty string when it fails but a string starting with 'error-', so only an
absolute path is accepted: writing into that string would create the files
under the web root.
- fetchObjectByElement() returns an object even when fetch() returned 0. The
object was then not loaded, and the file was stored at the root of the
directory of the module, out of any object. The constructor now throws, and
the endpoint answers the error with the same json contract than a successful
call so that the caller can show it, instead of a fatal error and an http 500.
- An attachment of the same name was silently overwritten, dol_move_uploaded_file()
being called with $allowoverwrite = 1 while the name was checked before the
ref of the object was added as a prefix. The check is done again on the final
name, and on the .noexe suffixed name too, which that function appends to an
executable file.
- An executable file was renamed with that .noexe suffix and then reported as an
error, while it was correctly stored.
- The endpoint called restrictedArea() with an empty feature when the element is
unknown, and the loop of that function then takes no branch at all and grants
the access with no check. It refuses before, with the same http code and the
same message than a refusal, so that a user cannot tell an object that exists
but is not allowed from an object that does not exist. The refusal of an
external user on an object of another third party answered a message of its
own, which allowed the same enumeration.
Comes with the tests of the path resolution, of the fallback of get_exdir(), of
the rejection of the error string of getMultidirOutput(), and of the file name
deduplication.
* FIX Remove the drop area from 13 cards where dropping a file is harmful
Two distinct groups, both of them removing a drop area that only produces a
result the user does not want.
7 pages have no "Attached files" tab at all, so a file dropped on them can be
reached by no screen: a fiscal year and its info page, an accounting model, a
webhook trigger history, an intracomm report, a bookcal calendar and its booking
list. Measured: 4 of them accepted the upload and wrote an orphan file, the
accounting model wrote it at the root of the directory of the module, out of any
object, and the 2 bookcal ones answered an http 500.
6 cards print their tabs inside their edit form, so the drop area covered that
form: a product, an expense report, a VAT payment, a social contribution, a loan
and a salary. Dropping a file there reloads the page, which discards what the
user is typing. A product and an expense report have a second call for the read
only view, so the drop area is only removed from the call of the edit branch;
the 4 others share a single call between both views, so the parameter is now
conditional.
* NEW Drag and drop a file on the 23 remaining cards
Every card of the core that owns an "Attached files" tab can now receive a file
by drag and drop, which was the case of 32 of them only. The 23 added here are
an asset, an event, a various payment, a customer payment, a contact, a donation,
a conference or booth, a shipment, a supplier payment, a leave request, an
evaluation, a job, a position, a skill, a knowledge record, a manufacturing
order, a partnership, a lot, a stock transfer, a task, a reception, a resource
and a workstation.
The tab bar of a leave request and of a resource is printed inside their edit
form, so the drop area is not enabled there: dropping a file reloads the page,
which would discard what the user is typing. The card of a supplier payment
prints its tab bar even when the object was not loaded, so the drop area is only
enabled when it is.
* FIX getMultidirOutput refuses an entity with no directory instead of falling back [skip-claudemd]
The previous revision of this PR fell back on the directory of the current
entity when the entity of the object had none, with a LOG_WARNING. On a
multicompany install that made a caller read, write and above all delete
files in the directory of another entity. Refuse instead: the function
already answers 'error-diroutput-not-defined-for-this-object' when the
module declares no directory at all, so the caller has one behaviour to
handle, not two.
The entity is cast to int, which is what the array index needs and what
silences the four PhanTypeMismatchDimFetchNullable this function reported.
---------
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
Data structure only, submitted standalone as asked by CONTRIBUTING.md, so it can be
reviewed and accepted before the code that uses it.
A contact can either carry its own address or reuse the address of its thirdparty.
Today the choice is guessed from the content of the address fields, which cannot tell
an empty address from a deliberate "use the thirdparty address". The new column stores
the choice explicitly.
0 = use the address of the contact
1 = use the address of the linked thirdparty
NULL = legacy resolution, the value for every existing record, so an existing
alternative address stays independent from its thirdparty address.
Code side: #38118.
* FIX: Restore PHPUnit 11 compatibility in CommonClassTest
onNotSuccessfulTest() overrode PHPUnit\Framework\TestCase's method with
an incompatible return type (void vs never), and the assertMatchesRegularExpression
back-compat shim for PHPUnit <8 now illegally overrides that method, which
PHPUnit 11 declares final. Both faults made every test extending
CommonClassTest fatal immediately on this environment's PHPUnit 11.5.55.
Pre-existing breakage, unrelated to the PDF mutualization work in this
branch — fixed here because Task 1's plan requires a passing
BuildDocTest.php baseline before any implementer is dispatched.
* NEW: Mutualize PDF header logo block into pdf_writeLogoOrCompanyName() (sponge, einstein, azur, muscadet)
* NEW: Mutualize PDF header logo block into pdf_writeLogoOrCompanyName() (cornas, standard, cyan, octopus, strato)
* FIX: Resolve $logodir before calling pdf_writeLogoOrCompanyName() in cornas, standard, cyan, octopus, strato
* NEW: Mutualize PDF header logo block into pdf_writeLogoOrCompanyName() (eratosthene, eagle_proforma, standard_asset)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* NEW: Mutualize PDF header logo block into pdf_writeLogoOrCompanyName() (aurore, zenith)
* FIX: Complete PHPUnit 11 compatibility fix in CommonClassTest::onNotSuccessfulTest
The prior compatibility fix (f29129aae43) resolved the two class-load
fatals but left onNotSuccessfulTest() calling getName()/getDataSetAsString(),
neither of which exists on PHPUnit 11's TestCase (renamed to name() and
dataSetAsStringWithData()/dataSetAsString()). The undefined-method error
was silently absorbed by PHPUnit's own exception handling, so every test
failure across the suite still ran and reported correctly, but the
method's entire diagnostic block (log tail, backtrace, DB info dump) never
executed. Verified with a throwaway reproduction test before and after:
the diagnostic ##[group] output was empty before this fix and complete
after it.
Found during the final whole-branch review of the PDF logo-block
mutualization branch; unrelated to that refactor but touches the same
file already modified by the earlier compatibility commit.
* FIX: Drop unneeded by-ref $pdf and rename $ltrdirection to $align in pdf_writeLogoOrCompanyName()
TCPDF objects are handles, so passing $pdf by reference bought nothing
and was inconsistent with every sibling function in this file
(pdf_pagehead, pdf_watermark, pdf_bank, pdf_pagefoot, ...), all of which
take $pdf by value. $ltrdirection was also a misnomer: three of the 14
callers pass the literal 'L' and one passes 'J' (justify) - none of those
is a text direction, it's an alignment. Renaming is call-site-transparent
(all 14 callers use positional arguments).
Found during the final whole-branch review of the PDF logo-block
mutualization branch.
* NEW: Document pdf_writeLogoOrCompanyName() helper in ChangeLog
Per repo convention (CLAUDE.md: "Always update ChangeLog for
significant changes"). Flagged during the final whole-branch review
of the PDF logo-block mutualization branch: a new reusable pdf_lib.php
function is part of the surface third-party PDF document-model modules
build against.
* restore commonclasstest
* fix
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
* NEW: Improve phpunit test coverage for Commande, Propal and Facture
Add line CRUD (addline/updateline/deleteLine) and total-consistency
assertions, reuse the specimen-invariant regression check (previously
only in FactureTest) across all three, and add a new integration test
covering the full Propal -> Commande -> Facture conversion chain
(thirdparty, notes, totals and object_linked propagation).
Two shared assertions were added to CommonClassTest for this:
assertLineTotalsMatchHeader() and assertMatchesFreshSpecimen().
* Update commande.class.php
* FIX PropalCommandeFactureWorkflowTest reads wrong invoice after createFromOrder
Facture::createFromOrder() returns a status flag (1/-1), not the new
invoice id, unlike Commande::createFromProposal(). The test was doing
fetch($result) with $result==1, which happened to no-op locally (no
invoice with rowid=1) but fetched an unrelated invoice in CI, causing
a spurious thirdparty mismatch. Use $facture->id, already set by
create() inside createFromOrder(), like every other caller of this
method does.
* NEW: Add phpunit test for Mo class
Add a unit-level test for the Mo (manufacturing order) class, which
had no direct test coverage yet (RestAPIMoTest.php only exercises
the REST API layer over HTTP, and its most interesting part -
produce/consume - is entirely commented out there).
Covers create() (including the automatic "to produce" line it
creates for the finished product), fetch, update, the
draft -> validated -> canceled -> validated status workflow
(validate/cancel/reopen, including the provisional ref being
replaced on validate()), and delete.
Uses a freshly created specimen Product for fk_product rather than a
random real catalog product: Mo::create() rejects kit/BOM products
unless ALLOW_USE_KITS_INTO_BOM_AND_MO is set, and a specimen product
is guaranteed not to be one.
The module is auto-activated in setUpBeforeClass() if not already
enabled, following the same pattern as the other recently added
tests (activation is real and persists after the test run, it is not
undone by the rollback in tearDownAfterClass - see comment in
setUpBeforeClass() for why).
* FIX MoTest crash in the full test suite (stale $db)
Same class of bug as StockTransferTest (see that commit for the full
analysis): modMrp depends on modBom, which itself depends on
modProduct, whose constructor queries the DB via Societe::useNPR().
If $db is stale/closed when this class's setUpBeforeClass() runs in
the full suite (all classes in one continuous process), activating
modMrp crashes on the dead connection the same way.
Reconnect $db (and refresh $mysoc->db/$user->db, which stashed their
own reference to the old connection at bootstrap) before calling
activateModule(), same pattern as StockTransferTest.
Verified the same way: closing $db and calling
MoTest::setUpBeforeClass() directly reproduces the crash without
this fix and is resolved with it.
* FIX MoTest: resync $mysoc/$user->db unconditionally
Same follow-up fix as StockTransferTest (see that commit for the full
analysis): the previous defensive-reconnect only refreshed
$mysoc->db/$user->db inside the branch where the global $db itself
was detected stale. $mysoc/$user can diverge from a healthy $db
independently (they stash their own ->db reference at bootstrap), so
resync them unconditionally instead.
Verified the same way: closing the original $db but reconnecting only
the global $db variable (leaving $mysoc->db pointing at the closed
one) reproduces the crash with the old code and is resolved with this
fix.
* NEW Centralize the stale-$db reconnect logic in CommonClassTest
Same follow-up as StockTransferTest (#39542): the defensive
reconnect-and-resync-$mysoc/$user logic was duplicated identically
across 3 test classes. Extract it into a shared
CommonClassTest::ensureDbIsConnected() helper so future test classes
that activate a module depending on modProduct don't need to
reimplement it, and so the fix can't drift between test files.
MoTest::setUpBeforeClass() now just calls self::ensureDbIsConnected()
before activateModule().
Re-verified the same way as before: closing the original $db but
reconnecting only the global $db variable (leaving $mysoc->db
pointing at the closed one) still reproduces the crash without this
fix and is resolved with it.
* Remove setUpBeforeClass from MoTest
Removed setUpBeforeClass method to simplify test setup.
* Update CommonClassTest.class.php
---------
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
Add the ability to merge two contacts, the same way third parties and members
can already be merged. The merged contact is absorbed by the current one, all
its satellite data is moved, then it is deleted.
Contact::mergeContact() orchestrates everything in a single transaction, using
the new CommonObject::commonReplaceContact() helper and the replaceContact()
static methods of Contact, ActionComm and User. A replaceContact hook lets the
external modules move their own data. No schema change, no migration.
Three specific traps of this table are handled:
- llx_element_contact.fk_socpeople references llx_socpeople only when
c_type_contact.source is 'external', and llx_user when it is 'internal', so
both queries filter on it. Without that filter the internal assignments of
the users would silently be moved to the merged contact.
- Contact::updateRoles(), called by update(), deletes then reinserts every
societe_contacts row of the contact from $this->roles. It is neutralised by
setting roles to null, and update() is called before the links are moved.
- Every table having a unique index on the contact id is deduplicated before
its update, so the update cannot violate it. $ignoreerrors is never used:
the FIXME of commonReplaceThirdparty() documents the links it loses.
The files are moved after the commit and not before, unlike mergeCompany() and
mergeMembers(): a failure after the move leaves them with the database rolled
back but the files already gone. A name collision renames the file instead of
losing it, and a failure is reported to the user instead of being ignored.
Access guards live in the class and not in the page, so the REST API, the
scheduled jobs and the modules benefit from them too. They cover both contacts,
fetch() by rowid applying neither an entity nor a permission filter: entity,
private contact of another user, external user, sales representative perimeter,
absorbing a shared contact into a private one, and a contact linked to a user
account (moving llx_user.fk_socpeople would let the email of that user be
rewritten from the contact card, so it requires the permission to create users).
Form::select_contact() gets a one line fix on the way: it accepts a $filter
parameter, documents it, forwards it to the ajax branch, but dropped it when
building the combo, so the merge popup could not exclude the current contact.
Known limitations, out of the scope of this pull request: merging contacts of
different entities is refused, Contact::update() neither writes url, no_email,
fk_parent nor import_key while its $nosyncuser parameter is declared but never
read, and with CONTACT_USE_SEARCH_TO_SELECT set the current contact is still
offered in the popup, contact/ajax/contact.php overwriting the filter it gets.
* 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.
The label of the automatic agenda event logged when a contact is modified is
built from $object->name, a property the Contact class does not declare and
fetch() never fills, so the name is always empty and PHP 8 reports an undefined
property. The CONTACT_CREATE branch a few lines above already uses
getFullName(), which is what this branch was meant to call too.
Also adds the @phan-var-force annotation the neighbouring branches carry.
Add tests for Don and PaymentDonation, which had no test coverage
yet: create/fetch/update/delete, the draft -> validated -> paid
status workflow (setValid/setPaid) and a separate self-contained
cancel test (set_cancel() has no status precondition, unlike
setValid()/setPaid()).
Note: create() never writes $this->status to the fk_statut column
(it always defaults to draft in DB) even though initAsSpecimen() sets
status=1 in memory - the test re-fetches to check the real status.
PaymentDonationTest also cross-checks Don::getRemainToPay() after a
full payment: unlike Tva::addPayment()/getSommePaiement() (see
TvaTest.php), PaymentDonation::create() and Don::getRemainToPay() do
agree on the same fk_donation link, so this is a genuine regression
check, not a workaround.
The module is auto-activated in setUpBeforeClass() if not already
enabled, following the same pattern as the other recently added
tests (activation is real and persists after the test run, it is not
undone by the rollback in tearDownAfterClass - see comment in
setUpBeforeClass() for why).
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
Add a test for RemiseCheque (chequereceipt / bank check remittance
slip), which had no test coverage yet.
RemiseCheque does not create a standalone record: it gathers existing
not-yet-remitted check payments (llx_bank rows with fk_type='CHQ' and
fk_bordereau=0) for a given bank account into a receipt. The fixture
therefore goes through the real path: create+validate an invoice, pay
it with a CHQ-coded payment, reconcile it to a bank account (this is
what produces the llx_bank row RemiseCheque::create() picks up), then
remit that check. Covers create/fetch/validate/delete.
Note: unlike most other classes in this suite, RemiseCheque::delete()/
updateAmount() return an "errno" convention (0=success, <0=error)
rather than the usual ">0=success" one used by create()/validate() in
the same class - see docblock and inline comments.
No module activation needed here: the cheque receipt feature is not
gated behind any separately activatable module (only the always-on
bank module).
Add CRUD tests (create/fetch/update/delete) for LoanSchedule and
PaymentLoan, which had no test coverage yet, each against a freshly
created specimen Loan. LoanScheduleTest also covers
calcMonthlyPayments() (pure amortization formula, no DB) with both
the zero-rate and standard amortization branches.
Note: PaymentLoan::create() is inconsistent with its own fetch()/
update(): it declares (and even cleans) $fk_loan and $fk_typepayment
like fetch()/update() use, but its INSERT statement actually reads
$chid and $paymenttype instead (left uncleaned). This matches the
real caller (loan/payment/payment.php), so the test sets chid/
paymenttype for create() - see class docblock in PaymentLoanTest.php.
The module is auto-activated in setUpBeforeClass() if not already
enabled, following the same pattern as the other recently added
tests (activation is real and persists after the test run, it is not
undone by the rollback in tearDownAfterClass - see comment in
setUpBeforeClass() for why).
Continuing the audit of commonly-used but untested functions in
functions.lib.php.
- testDolPrintSize: the 10240-byte threshold that decides whether
$shortvalue actually converts to Kilobytes, and the short vs long
unit label.
- testDolGetFirstLastname: all 6 documented $nameorder values (0-5),
including the "if defined else the other" fallback behavior of
3 and 5, and the -1 auto mode.
- testJsonOrUnserialize: valid JSON (object and array forms), a
legacy PHP-serialized string falling back to unserialize(), and a
string that is neither (returns false; the resulting unserialize()
warning is expected and suppressed in the test).
- testPictoFromLangcode: empty input, the 'auto' special case, a
hardcoded special-cased language code (fr_CA), the generic
'xx_YY' -> country-part flag resolution, a bare country code, the
$notitlealt flag, and how a 'class="..."' in $moreatt merges into
the span's own class instead of becoming a separate attribute.
All expected values were verified empirically against the real
function outputs before being written into the test.
CommandeFournisseur::getTooltipContentArray() already adds a "Supplier"
line (thirdparty getNomUrl) to the getNomUrl tooltip, but
FactureFournisseur and SupplierProposal do not. Hovering a supplier
invoice or supplier proposal link therefore never shows which thirdparty
it belongs to, an inconsistency between three otherwise-similar objects.
Align both with CommandeFournisseur: add the same $datas['supplier']
entry, right after RefSupplier, honoring the 'nofetch' param.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* NEW : Derive ModuleBuilder status labels from arrayofkeyval
LibStatut() in the ModuleBuilder object template hardcoded the
Draft/Enabled/Disabled labels, which diverged from the labels defined
in the 'status' field arrayofkeyval used by the list filter and the
select. Build labelStatus/labelStatusShort from arrayofkeyval so the
badge, the filter and the select all show the same configurable labels.
Add ModuleBuilderTemplateConventionsTest covering the derivation.
Next: normalize trigger codes to MYMODULE_MYOBJECT_ACTION in class line 637 + typo line 50, sql/data.sql lines 27/30-32, myobject_card.php line 378; extend the test with trigger assertions.
* NEW : Use explicit MODULE_OBJECT_ACTION trigger naming in template
The ModuleBuilder object template emitted the validate trigger as the
generic MYOBJECT_VALIDATE, inconsistent with the MYMODULE_MYOBJECT
prefix already advertised by $TRIGGER_PREFIX and used by the
unvalidate/cancel/reopen/sentbymail trigger codes. Normalize the
validate trigger, the seed data.sql agenda triggers and the close
notification example to the MYMODULE_MYOBJECT_<ACTION> policy so
generated modules expose clear, non-ambiguous trigger codes. Also fix a
typo in the $TRIGGER_PREFIX comment.
Extend ModuleBuilderTemplateConventionsTest with trigger naming checks.
Next: ChangeLog entry covering both features, then dolibarr-audit + functional verification, then push to remote quentin and run pr-review-v2.
* DOC : Add ChangeLog entries for ModuleBuilder status labels and triggers
Next: dolibarr-audit on both features, then functional verification, then push to remote quentin and run pr-review-v2.
* FIX : Guard ModuleBuilder LibStatut against undefined status key
Hardening from audit: when LibStatut() is called with a status value
that is not present in the 'status' field arrayofkeyval, reading
labelStatus/labelStatusShort raised an undefined-array-key warning under
PHP 8. Default both labels to an empty string, which dolGetStatus
already accepts, so the badge degrades gracefully to the status code.
Next: push branch to remote quentin and run dolibarr-pr-review-v2 on the full diff vs develop.
* CHORE Drop the files CONTRIBUTING forbids to edit in a PR [skip-claudemd]
ChangeLog is generated from the commit messages at release time, and the
language files other than en_US are synced from Transifex.
---------
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
Co-authored-by: Alexandre SPANGARO <aspangaro.dolibarr@gmail.com>
getElementProperties() is the central registry used across the
codebase (fetchObjectByElement(), generic links/extrafields, document
generation...) to resolve an element type string into its module,
classpath, classfile, classname, table and parent element, but it had
no test coverage despite being a ~480 line function with about 60
special-case branches plus several generic fallback paths.
Covers the representative paths rather than every branch:
- the generic completion of classfile/classname from subelement
(project)
- the 'myobject@mymodule' external-module syntax, including the
surprising fact that table_element keeps the raw '@'-string
- the 'myobject_mysubobject' syntax combined with a dedicated case
branch overriding module (project_task)
- the generic '...det' object-line fallback for an unknown module,
including the non-capitalized classname it produces (myobjectdet)
- a real '...det' case where the generic fallback runs first and a
dedicated branch only adds parent_element on top (contratdet)
- a real '...det' case where the dedicated branch instead overrides
classpath and classname set by the generic fallback (facturedet)
- the action/actioncomm special case, where table_element differs
from the raw input element
* Qual: Use dolBuildUrl() instead of manual urlencode() concatenation in ecm
Replaces manual '?key='.urlencode($val).'&key2='.urlencode($val2)
string-building with dolBuildUrl($path, $params) across the ECM
directory-tree/file-manager code, for consistency with the rest of
the codebase (see htdocs/core/lib/ecm.lib.php, which already uses
this pattern for the same page) and to benefit from dolBuildUrl()'s
buildurl hook.
- core/ajax/ajaxdirtree.php: the dir_card.php edit link built from
the sql tree loop.
- ecm/dir_card.php: the edit/add-section action buttons and the two
delete confirmation URLs. Also switches the three buttons that used
to manually concatenate '&token='.newToken() to dolBuildUrl()'s own
$addtoken parameter.
- ecm/class/ecmfiles.class.php: EcmFiles::getNomUrl()'s document.php
and file_card.php URLs.
- ecm/tpl/enablefiletreeajax.tpl.php: the ajaxdirtree.php script URL
and the ajaxdirpreview.php URL. The token here intentionally stays
currentToken() (not dolBuildUrl()'s own newToken()-based
$addtoken), per the existing comment: ajaxdirtree.php has
NOTOKENRENEWAL defined, so the token must match the one already
valid on the calling page. $paramwithoutsection is a pre-built raw
query-string fragment from an external caller and is appended as-is
after the dolBuildUrl() result rather than folded into it.
Verified all five refactored URL-building expressions produce byte-
identical output to the original code for representative inputs
(including values with '/', '&' and spaces), except for query
parameter order (which has no effect) and one real, minor pre-
existing bug this incidentally fixes: the delete-section confirm URL
in dir_card.php was building '&module='.$module without urlencode(),
now correctly encoded by dolBuildUrl()/http_build_query().
Could not do a live browser check (no Chrome available for Playwright
in this environment) - verified via php -l, phpcs, and a standalone
script comparing old vs new output for each call site instead.
* Qual: Use dolBuildUrl() instead of manual urlencode() concatenation in filemanager.tpl.php
Same refactor as the previous commit, applied to the 7 remaining
manually-concatenated URLs in core/tpl/filemanager.tpl.php (used by
the ECM/medias file manager): the delete-file/delete-section/
convert-to-webp confirm URLs, the create-directory and refresh-list
toolbar buttons (now using dolBuildUrl()'s $addtoken instead of a
manual '&token='.newToken()), the two generate-webp buttons, and the
"Root" link.
$websitekeyandpageid is kept as a helper to build the raw sub-query
string embedded once (single-encoded) as the create-directory
button's 'backtopage' value - it is not itself passed to dolBuildUrl.
Verified all 7 refactored URL-building expressions produce the same
query parameters as the original code for representative inputs
(compared as parsed, order-independent query strings, since
http_build_query() does not preserve insertion order the same way as
the original manual concatenation), including one case
(convertimgwebp confirm with sortfield/sortorder) where the original
code had a harmless but sloppy leading '?&' that dolBuildUrl() no
longer produces.
* fix
* Qual: Use dolBuildUrl() instead of manual urlencode() concatenation in index_auto.php
Same refactor as the previous commits, applied to the 4 manually-
concatenated URLs in ecm/index_auto.php: the delete-file and
delete-section confirm URLs, the refresh-list toolbar link, and the
per-directory link in the auto-directories list.
Verified all 4 refactored URL-building expressions produce the same
query parameters as the original code for representative inputs
(including an empty-module/empty-section case for the refresh link,
and values with '/' and spaces for the others).
* Qual: Use dolBuildUrl() instead of manual urlencode() concatenation in ecm (file_card, dir_add_card, index_medias)
Same refactor as the previous commits, applied to the remaining
manually-concatenated URLs in:
- ecm/file_card.php: the cancel and rename-file redirects, the
internal download link (document.php), the delete-file confirm URL
and the edit button.
- ecm/dir_add_card.php: the delete-section confirm URL and the delete
button (now using dolBuildUrl()'s $addtoken instead of a manual
'&token='.newToken()).
- ecm/index_medias.php: the $backtopage URL used by
core/actions_linkedfiles.inc.php after a confirm_deletefile.
Left ecm/search.php's '$param = "§ion=".urlencode($section)'
alone: it is a raw query-string fragment (starting with '&', no
leading path) passed into FormFile::list_of_documents(), not a
base+params URL build, so it does not fit the dolBuildUrl($path,
$params) shape - same reasoning as $paramwithoutsection in the
already-refactored enablefiletreeajax.tpl.php.
Verified all 8 refactored URL-building expressions produce the same
query parameters as the original code for representative inputs
(order-independent comparison, since http_build_query() does not
preserve the original insertion order).
