Skip to content

Commit e10893e

Browse files
fabiodalez-devfabiodalez-dev
committed
fix: address CodeRabbit review + add sharing E2E tests
- SettingsController: dedup/canonicalize sharing slugs (array_unique + lowercase) - BookRepository: toPlainTextDescription() helper handles <p>, <br>, entities instead of raw strip_tags() - DigitalLibraryPlugin: atomic hook registration with INSERT...ON DUPLICATE KEY UPDATE, replacing race-prone SELECT+INSERT - schema.sql + migrate_0.5.0.sql: add UNIQUE KEY uk_plugin_hook_callback on plugin_hooks (plugin_id, hook_name, callback_class, callback_method) - frontend-buttons.php: add rel="noopener noreferrer" on target="_blank" link - frontend-pdf-viewer.php + DigitalLibraryPlugin + frontend-buttons.php: parse_url() before pathinfo() to handle query strings in URLs - locale/en_US.json + de_DE.json: remove duplicate keys (Anteprima, Condividi, Condividi su Facebook, Condividi su WhatsApp, Link copiato!) - README.md: update Digital Library plugin version 1.2.0 → 1.3.0 - settings/index.php: deep-linkable tabs via ?tab= and #hash with popstate - Add social-sharing.spec.js: 9 E2E tests covering settings, frontend buttons, OG meta tags, provider toggle, and share URL correctness
1 parent 4ff3781 commit e10893e

File tree

12 files changed

+398
-72
lines changed

12 files changed

+398
-72
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ curl "http://yoursite.com/api/sru?operation=searchRetrieve&query=bath.isbn=97888
630630
- **Retry logic** with exponential backoff
631631
- **Error logging** and debugging tools
632632

633-
### 4. Digital Library (`digital-library-v1.2.0.zip`)
633+
### 4. Digital Library (`digital-library-v1.3.0.zip`)
634634
- **eBook support** (PDF, ePub) with download tracking
635635
- **Audiobook streaming** (MP3, M4A, OGG) with HTML5 player
636636
- **Per-item digital asset management** (unlimited files per book)

‎app/Controllers/SettingsController.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,8 @@ public function updateSharingSettings(Request $request, Response $response, mysq
905905
if (!is_array($selected)) {
906906
$selected = [];
907907
}
908-
$valid = array_values(array_intersect($selected, $allowedSlugs));
908+
$selected = array_map(static fn($s) => strtolower(trim((string) $s)), $selected);
909+
$valid = array_values(array_intersect($allowedSlugs, array_unique($selected)));
909910
$value = implode(',', $valid);
910911

911912
$repository->set('sharing', 'enabled_providers', $value);

‎app/Models/BookRepository.php‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public function getById(int $id): ?array
9898
&& $row['descrizione_plain'] === null
9999
&& !empty($row['descrizione'])
100100
) {
101-
$plain = strip_tags((string)$row['descrizione']);
101+
$plain = $this->toPlainTextDescription((string)$row['descrizione']);
102102
$upd = $this->db->prepare("UPDATE libri SET descrizione_plain = ? WHERE id = ?");
103103
if ($upd) {
104104
$upd->bind_param('si', $plain, $id);
@@ -298,7 +298,7 @@ public function createBasic(array $data): int
298298
}
299299
if ($this->hasColumn('descrizione_plain')) {
300300
$raw = $data['descrizione'] ?? null;
301-
$addField('descrizione_plain', 's', $raw !== null && $raw !== '' ? strip_tags((string)$raw) : $raw);
301+
$addField('descrizione_plain', 's', $this->toPlainTextDescription($raw));
302302
}
303303
if ($this->hasColumn('parole_chiave')) {
304304
$addField('parole_chiave', 's', $data['parole_chiave'] ?? null);
@@ -628,7 +628,7 @@ public function updateBasic(int $id, array $data): bool
628628
}
629629
if ($this->hasColumn('descrizione_plain')) {
630630
$raw = $data['descrizione'] ?? null;
631-
$addSet('descrizione_plain', 's', $raw !== null && $raw !== '' ? strip_tags((string)$raw) : $raw);
631+
$addSet('descrizione_plain', 's', $this->toPlainTextDescription($raw));
632632
}
633633
if ($this->hasColumn('parole_chiave')) {
634634
$addSet('parole_chiave', 's', $data['parole_chiave'] ?? null);
@@ -1172,6 +1172,18 @@ private function resolveGenreHierarchy(array &$row): void
11721172
}
11731173
}
11741174

1175+
private function toPlainTextDescription(?string $html): ?string
1176+
{
1177+
if ($html === null || $html === '') {
1178+
return $html;
1179+
}
1180+
$text = preg_replace('/<(\/p|br\s*\/?)>/i', "\n", $html);
1181+
$text = html_entity_decode(strip_tags((string) $text), ENT_QUOTES | ENT_HTML5, 'UTF-8');
1182+
$text = preg_replace("/[ \t]+/", ' ', (string) $text);
1183+
$text = preg_replace("/\n{3,}/", "\n\n", (string) $text);
1184+
return trim((string) $text);
1185+
}
1186+
11751187
private static array $columnCacheByDb = [];
11761188
private function hasColumn(string $name): bool
11771189
{

‎app/Views/settings/index.php‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -713,17 +713,25 @@ function activateTab(tabName) {
713713
});
714714
});
715715

716-
// Check URL hash on page load
716+
// Check URL on page load: hash takes priority, then ?tab= query param
717717
const hash = window.location.hash.substring(1);
718-
if (hash && document.querySelector(`[data-settings-tab="${hash}"]`)) {
719-
activateTab(hash);
718+
const qTab = new URL(window.location.href).searchParams.get('tab');
719+
const initialTab = (hash || qTab || '');
720+
if (initialTab && document.querySelector(`[data-settings-tab="${initialTab}"]`)) {
721+
activateTab(initialTab);
722+
// Sync URL so both ?tab= and # are consistent
723+
const url = new URL(window.location.href);
724+
url.searchParams.set('tab', initialTab);
725+
url.hash = initialTab;
726+
window.history.replaceState({}, '', url.toString());
720727
}
721728

722729
// Handle browser back/forward
723-
window.addEventListener('hashchange', () => {
724-
const currentHash = window.location.hash.substring(1);
725-
if (currentHash && document.querySelector(`[data-settings-tab="${currentHash}"]`)) {
726-
activateTab(currentHash);
730+
window.addEventListener('popstate', () => {
731+
const url = new URL(window.location.href);
732+
const tab = url.hash.substring(1) || url.searchParams.get('tab') || '';
733+
if (tab && document.querySelector(`[data-settings-tab="${tab}"]`)) {
734+
activateTab(tab);
727735
}
728736
});
729737

‎installer/database/migrations/migrate_0.5.0.sql‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,19 @@ DEALLOCATE PREPARE stmt;
3232
INSERT IGNORE INTO system_settings (category, setting_key, setting_value, description, updated_at)
3333
VALUES ('sharing', 'enabled_providers', 'facebook,x,whatsapp,email',
3434
'Enabled social sharing providers on book detail page', NOW());
35+
36+
-- =============================================================================
37+
-- Add unique index on plugin_hooks for atomic upsert registration
38+
-- =============================================================================
39+
-- Prevents duplicate hook rows from concurrent registerHooks() calls.
40+
41+
SET @idx_exists = (SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS
42+
WHERE TABLE_SCHEMA = DATABASE()
43+
AND TABLE_NAME = 'plugin_hooks'
44+
AND INDEX_NAME = 'uk_plugin_hook_callback');
45+
SET @sql = IF(@idx_exists = 0,
46+
'ALTER TABLE plugin_hooks ADD UNIQUE KEY uk_plugin_hook_callback (plugin_id, hook_name, callback_class, callback_method)',
47+
'SELECT 1');
48+
PREPARE stmt FROM @sql;
49+
EXECUTE stmt;
50+
DEALLOCATE PREPARE stmt;

‎installer/database/schema.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,7 @@ CREATE TABLE `plugin_hooks` (
545545
`is_active` tinyint(1) NOT NULL DEFAULT '1' COMMENT 'Whether hook is active',
546546
`created_at` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP,
547547
PRIMARY KEY (`id`),
548+
UNIQUE KEY `uk_plugin_hook_callback` (`plugin_id`,`hook_name`,`callback_class`,`callback_method`),
548549
KEY `idx_hook_name` (`hook_name`,`priority`),
549550
KEY `idx_plugin_id` (`plugin_id`),
550551
KEY `idx_active` (`is_active`),

‎locale/de_DE.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4039,13 +4039,10 @@
40394039
"Usa il controllo schermo intero del viewer o del browser": "Verwenden Sie die Vollbildsteuerung des Viewers oder Browsers",
40404040
"Condivisione": "Teilen",
40414041
"Seleziona i pulsanti di condivisione da mostrare nella pagina del libro.": "Wählen Sie die Teilen-Schaltflächen aus, die auf der Buchseite angezeigt werden sollen.",
4042-
"Anteprima": "Vorschau",
40434042
"Nessun pulsante selezionato": "Keine Schaltflächen ausgewählt",
40444043
"Salva impostazioni condivisione": "Freigabeeinstellungen speichern",
40454044
"Impostazioni di condivisione aggiornate.": "Freigabeeinstellungen aktualisiert.",
4046-
"Condividi su Facebook": "Auf Facebook teilen",
40474045
"Condividi su X": "Auf X teilen",
4048-
"Condividi su WhatsApp": "Auf WhatsApp teilen",
40494046
"Condividi su Telegram": "Auf Telegram teilen",
40504047
"Condividi su LinkedIn": "Auf LinkedIn teilen",
40514048
"Condividi su Reddit": "Auf Reddit teilen",
@@ -4058,7 +4055,5 @@
40584055
"Condividi su LINE": "Auf LINE teilen",
40594056
"Invia via SMS": "Per SMS senden",
40604057
"Invia per email": "Per E-Mail senden",
4061-
"Copia link": "Link kopieren",
4062-
"Link copiato!": "Link kopiert!",
4063-
"Condividi": "Teilen"
4058+
"Copia link": "Link kopieren"
40644059
}

‎locale/en_US.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4039,13 +4039,10 @@
40394039
"Usa il controllo schermo intero del viewer o del browser": "Use the viewer or browser fullscreen control",
40404040
"Condivisione": "Sharing",
40414041
"Seleziona i pulsanti di condivisione da mostrare nella pagina del libro.": "Select which share buttons to show on the book page.",
4042-
"Anteprima": "Preview",
40434042
"Nessun pulsante selezionato": "No buttons selected",
40444043
"Salva impostazioni condivisione": "Save sharing settings",
40454044
"Impostazioni di condivisione aggiornate.": "Sharing settings updated.",
4046-
"Condividi su Facebook": "Share on Facebook",
40474045
"Condividi su X": "Share on X",
4048-
"Condividi su WhatsApp": "Share on WhatsApp",
40494046
"Condividi su Telegram": "Share on Telegram",
40504047
"Condividi su LinkedIn": "Share on LinkedIn",
40514048
"Condividi su Reddit": "Share on Reddit",
@@ -4058,7 +4055,5 @@
40584055
"Condividi su LINE": "Share on LINE",
40594056
"Invia via SMS": "Send via SMS",
40604057
"Invia per email": "Send by email",
4061-
"Copia link": "Copy link",
4062-
"Link copiato!": "Link copied!",
4063-
"Condividi": "Share"
4058+
"Copia link": "Copy link"
40644059
}

‎storage/plugins/digital-library/DigitalLibraryPlugin.php‎

Lines changed: 26 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -196,54 +196,35 @@ private function registerHooks(): void
196196
];
197197

198198
foreach ($hooks as $hook) {
199-
// Check if hook already exists (include callback_method to support
200-
// multiple callbacks on the same hook_name, e.g. audio + PDF on digital_player)
199+
// Atomic upsert — relies on UNIQUE KEY uk_plugin_hook_callback
200+
// (plugin_id, hook_name, callback_class, callback_method)
201201
$stmt = $this->db->prepare("
202-
SELECT id FROM plugin_hooks
203-
WHERE plugin_id = ? AND hook_name = ? AND callback_method = ?
202+
INSERT INTO plugin_hooks
203+
(plugin_id, hook_name, callback_class, callback_method, priority, is_active)
204+
VALUES (?, ?, ?, ?, ?, ?)
205+
ON DUPLICATE KEY UPDATE
206+
priority = VALUES(priority),
207+
is_active = VALUES(is_active)
204208
");
205209
if (!$stmt) {
206-
\App\Support\SecureLogger::error('DigitalLibraryPlugin: prepare() failed for hook check', ['hook' => $hook['hook_name']]);
210+
\App\Support\SecureLogger::error('DigitalLibraryPlugin: prepare() failed for hook upsert', ['hook' => $hook['hook_name']]);
207211
continue;
208212
}
209-
$stmt->bind_param("iss", $this->pluginId, $hook['hook_name'], $hook['callback_method']);
210-
$stmt->execute();
211-
$result = $stmt->get_result();
212-
if (!$result instanceof \mysqli_result) {
213-
\App\Support\SecureLogger::error('DigitalLibraryPlugin: get_result() failed for hook check', ['hook' => $hook['hook_name']]);
214-
$stmt->close();
215-
continue;
216-
}
217-
218-
if ($result->num_rows === 0) {
219-
$stmt->close(); // close SELECT stmt before reassignment
220-
// Insert new hook
221-
$stmt = $this->db->prepare("
222-
INSERT INTO plugin_hooks
223-
(plugin_id, hook_name, callback_class, callback_method, priority, is_active)
224-
VALUES (?, ?, ?, ?, ?, ?)
225-
");
226-
if (!$stmt) {
227-
\App\Support\SecureLogger::error('DigitalLibraryPlugin: prepare() failed for hook insert', ['hook' => $hook['hook_name']]);
228-
continue;
229-
}
230-
$stmt->bind_param(
231-
"isssii",
232-
$this->pluginId,
233-
$hook['hook_name'],
234-
$hook['callback_class'],
235-
$hook['callback_method'],
236-
$hook['priority'],
237-
$hook['is_active']
238-
);
239-
if (!$stmt->execute()) {
240-
\App\Support\SecureLogger::error('[Digital Library] Hook insert failed', [
241-
'hook' => $hook['hook_name'],
242-
'error' => $stmt->error,
243-
]);
244-
}
213+
$stmt->bind_param(
214+
"isssii",
215+
$this->pluginId,
216+
$hook['hook_name'],
217+
$hook['callback_class'],
218+
$hook['callback_method'],
219+
$hook['priority'],
220+
$hook['is_active']
221+
);
222+
if (!$stmt->execute()) {
223+
\App\Support\SecureLogger::error('[Digital Library] Hook upsert failed', [
224+
'hook' => $hook['hook_name'],
225+
'error' => $stmt->error,
226+
]);
245227
}
246-
247228
$stmt->close();
248229
}
249230
}
@@ -320,8 +301,9 @@ public function renderAudioPlayer(array $book): void
320301
*/
321302
public function renderPdfViewer(array $book): void
322303
{
323-
$fileUrl = $book['file_url'] ?? '';
324-
if (!empty($fileUrl) && strtolower(pathinfo($fileUrl, PATHINFO_EXTENSION)) === 'pdf') {
304+
$fileUrl = (string) ($book['file_url'] ?? '');
305+
$filePath = (string) (parse_url($fileUrl, PHP_URL_PATH) ?? '');
306+
if ($filePath !== '' && strtolower(pathinfo($filePath, PATHINFO_EXTENSION)) === 'pdf') {
325307
include __DIR__ . '/views/frontend-pdf-viewer.php';
326308
}
327309
}

‎storage/plugins/digital-library/views/frontend-buttons.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515
return;
1616
}
1717

18-
$isPdf = $hasEbook && strtolower(pathinfo($book['file_url'] ?? '', PATHINFO_EXTENSION)) === 'pdf';
18+
$ebookPath = parse_url($book['file_url'] ?? '', PHP_URL_PATH) ?: ($book['file_url'] ?? '');
19+
$isPdf = $hasEbook && strtolower(pathinfo($ebookPath, PATHINFO_EXTENSION)) === 'pdf';
1920
?>
2021

2122
<?php if ($hasEbook && $isPdf): ?>
@@ -41,6 +42,7 @@ class="btn btn-outline-secondary btn-lg"
4142
<!-- Non-PDF (ePub etc.): open in new tab, no download attribute -->
4243
<a href="<?= htmlspecialchars(url($book['file_url']), ENT_QUOTES, 'UTF-8') ?>"
4344
target="_blank"
45+
rel="noopener noreferrer"
4446
class="btn btn-outline-danger btn-lg"
4547
title="<?= __("Scarica l'eBook in formato digitale") ?>">
4648
<i class="fas fa-file-pdf me-2"></i>

0 commit comments

Comments
 (0)