Skip to content

Commit 1399c5b

Browse files
thorstenthorsten
committed
refactor: improved code and added more tests
1 parent 6eb4e38 commit 1399c5b

File tree

2 files changed

+183
-22
lines changed

2 files changed

+183
-22
lines changed

‎phpmyfaq/src/phpMyFAQ/Bookmark.php‎

Lines changed: 86 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,25 @@
2121

2222
/**
2323
* Class Bookmark
24-
*
25-
* @package phpMyFAQ
2624
*/
27-
readonly class Bookmark
25+
class Bookmark
2826
{
27+
/**
28+
* @var array<int, object>|null
29+
*/
30+
private ?array $bookmarkCache;
31+
2932
/**
3033
* Constructor.
3134
*
3235
* @param Configuration $configuration Configuration object
3336
* @param CurrentUser $currentUser CurrentUser object
3437
*/
35-
public function __construct(private Configuration $configuration, private CurrentUser $currentUser)
36-
{
38+
public function __construct(
39+
private readonly Configuration $configuration,
40+
private readonly CurrentUser $currentUser
41+
) {
42+
$this->bookmarkCache = null;
3743
}
3844

3945
/**
@@ -44,10 +50,17 @@ public function __construct(private Configuration $configuration, private Curren
4450
*/
4551
public function isFaqBookmark(int $faqId): bool
4652
{
53+
if ($faqId <= 0) {
54+
return false;
55+
}
56+
4757
$bookmarks = $this->getAll();
58+
if ($bookmarks === []) {
59+
return false;
60+
}
4861

4962
foreach ($bookmarks as $bookmark) {
50-
if ((int) $bookmark->faqid === $faqId) {
63+
if (isset($bookmark->faqid) && (int) $bookmark->faqid === $faqId) {
5164
return true;
5265
}
5366
}
@@ -56,35 +69,54 @@ public function isFaqBookmark(int $faqId): bool
5669
}
5770

5871
/**
59-
* Saves a given Faq to the bookmark collection of the current user.
72+
* Saves a given FAQ to the bookmark collection of the current user.
6073
*
6174
* @param int $faqId ID of the Faq
6275
*/
6376
public function add(int $faqId): bool
6477
{
78+
if ($faqId <= 0) {
79+
return false;
80+
}
81+
6582
$query = sprintf(
66-
"INSERT INTO %sfaqbookmarks(userid, faqid) VALUES (%d, %d)",
83+
'INSERT INTO %sfaqbookmarks(userid, faqid) VALUES (%d, %d)',
6784
Database::getTablePrefix(),
6885
$this->currentUser->getUserId(),
6986
$faqId
7087
);
71-
return (bool) $this->configuration->getDb()->query($query);
88+
89+
$result = (bool) $this->configuration->getDb()->query($query);
90+
91+
if ($result) {
92+
$this->bookmarkCache = null;
93+
}
94+
95+
return $result;
7296
}
7397

7498
/**
7599
* Gets all bookmarks from the current user.
76100
*
77-
* @return array<int>
101+
* @return array<int, object> List of DB result objects each containing ->faqid
78102
*/
79103
public function getAll(): array
80104
{
105+
if ($this->bookmarkCache !== null) {
106+
return $this->bookmarkCache;
107+
}
108+
81109
$query = sprintf(
82110
'SELECT faqid FROM %sfaqbookmarks WHERE userid = %d',
83111
Database::getTablePrefix(),
84112
$this->currentUser->getUserId()
85113
);
86114
$result = $this->configuration->getDb()->query($query);
87-
return $this->configuration->getDb()->fetchAll($result);
115+
$data = $this->configuration->getDb()->fetchAll($result);
116+
117+
$this->bookmarkCache = is_array($data) ? $data : [];
118+
119+
return $this->bookmarkCache;
88120
}
89121

90122
/**
@@ -94,19 +126,28 @@ public function getAll(): array
94126
*/
95127
public function remove(int $faqId): bool
96128
{
129+
if ($faqId <= 0) {
130+
return false;
131+
}
132+
97133
$query = sprintf(
98134
'DELETE FROM %sfaqbookmarks WHERE userid = %d AND faqid = %d',
99135
Database::getTablePrefix(),
100136
$this->currentUser->getUserId(),
101137
$faqId
102138
);
103139

104-
return (bool) $this->configuration->getDb()->query($query);
140+
$result = (bool) $this->configuration->getDb()->query($query);
141+
142+
if ($result) {
143+
$this->bookmarkCache = null;
144+
}
145+
146+
return $result;
105147
}
106148

107149
/**
108150
* Removes all bookmarks from the current user.
109-
*
110151
*/
111152
public function removeAll(): bool
112153
{
@@ -116,9 +157,18 @@ public function removeAll(): bool
116157
$this->currentUser->getUserId()
117158
);
118159

119-
return (bool) $this->configuration->getDb()->query($query);
160+
$result = (bool) $this->configuration->getDb()->query($query);
161+
162+
if ($result) {
163+
$this->bookmarkCache = null;
164+
}
165+
166+
return $result;
120167
}
121168

169+
/**
170+
* @return array<int, array{url:string,title:string,id:int,answer:string}>
171+
*/
122172
public function getBookmarkList(): array
123173
{
124174
$bookmarks = $this->getAll();
@@ -132,27 +182,41 @@ public function getBookmarkList(): array
132182
$list = [];
133183

134184
foreach ($bookmarks as $bookmark) {
135-
$faq->getFaq((int) $bookmark->faqid);
185+
if (!isset($bookmark->faqid)) {
186+
continue;
187+
}
188+
$faqId = (int) $bookmark->faqid;
189+
if ($faqId <= 0) {
190+
continue;
191+
}
192+
193+
$faq->getFaq($faqId);
136194
$faqData = $faq->faqRecord;
137195

196+
if (empty($faqData['id'])) {
197+
continue;
198+
}
199+
200+
$categoryId = $category->getCategoryIdFromFaq($faqData['id']);
138201
$url = sprintf(
139202
'%sindex.php?action=faq&id=%d&cat=%d&artlang=%s',
140203
$this->configuration->getDefaultUrl(),
141-
$faqData['id'],
142-
$category->getCategoryIdFromFaq($faqData['id']),
143-
$faqData['lang']
204+
(int) $faqData['id'],
205+
$categoryId,
206+
$faqData['lang'] ?? ''
144207
);
145208

146209
$link = new Link($url, $this->configuration);
147-
$link->text = Strings::htmlentities($faqData['title']);
210+
$title = (string) ($faqData['title'] ?? '');
211+
$link->text = Strings::htmlentities($title);
148212
$link->itemTitle = $link->text;
149213
$link->tooltip = $link->text;
150214

151215
$list[] = [
152216
'url' => $link->toString(),
153-
'title' => htmlspecialchars_decode((string) $faqData['title']),
154-
'id' => $faqData['id'],
155-
'answer' => $faqData['content']
217+
'title' => htmlspecialchars_decode($title),
218+
'id' => (int) $faqData['id'],
219+
'answer' => (string) ($faqData['content'] ?? '')
156220
];
157221
}
158222

‎tests/phpMyFAQ/BookmarkTest.php‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,4 +88,101 @@ public function testRenderBookmarkTree(): void
8888
// Clean up
8989
$this->bookmark->remove(1);
9090
}
91+
92+
public function testAddInvalidId(): void
93+
{
94+
$this->bookmark->removeAll();
95+
$this->assertFalse($this->bookmark->add(0));
96+
$this->assertFalse($this->bookmark->add(-5));
97+
$this->assertSame([], $this->bookmark->getAll());
98+
}
99+
100+
public function testIsFaqBookmarkWithInvalidIdReturnsFalse(): void
101+
{
102+
$this->assertFalse($this->bookmark->isFaqBookmark(0));
103+
$this->assertFalse($this->bookmark->isFaqBookmark(-10));
104+
}
105+
106+
public function testRemoveInvalidId(): void
107+
{
108+
$this->assertFalse($this->bookmark->remove(0));
109+
$this->assertFalse($this->bookmark->remove(-2));
110+
}
111+
112+
public function testRemoveNotExistingBookmark(): void
113+
{
114+
$this->bookmark->removeAll();
115+
$this->assertTrue($this->bookmark->remove(99999));
116+
}
117+
118+
public function testRemoveAll(): void
119+
{
120+
$this->bookmark->removeAll();
121+
$this->bookmark->add(1);
122+
$this->bookmark->add(1); // evtl. Duplikat, aber sollte nicht schaden für den Test der Löschung
123+
$this->assertTrue($this->bookmark->removeAll());
124+
$this->assertSame([], $this->bookmark->getAll());
125+
}
126+
127+
public function testGetBookmarkListEmpty(): void
128+
{
129+
$this->bookmark->removeAll();
130+
$list = $this->bookmark->getBookmarkList();
131+
$this->assertIsArray($list);
132+
$this->assertCount(0, $list);
133+
}
134+
135+
public function testCacheInvalidationOnAdd(): void
136+
{
137+
$this->bookmark->removeAll();
138+
// Initialer Aufruf füllt Cache (leer)
139+
$this->bookmark->getAll();
140+
$reflection = new \ReflectionClass(Bookmark::class);
141+
$prop = $reflection->getProperty('bookmarkCache');
142+
$prop->setAccessible(true);
143+
$cached = $prop->getValue($this->bookmark);
144+
$this->assertIsArray($cached);
145+
146+
// Add invalidiert den Cache
147+
$this->bookmark->add(1);
148+
$afterAddCache = $prop->getValue($this->bookmark);
149+
$this->assertNull($afterAddCache, 'Cache sollte nach add() invalidiert sein');
150+
151+
// Neuer getAll-Aufruf baut Cache wieder auf
152+
$all = $this->bookmark->getAll();
153+
$this->assertNotNull($prop->getValue($this->bookmark));
154+
$this->assertCount(1, $all);
155+
156+
// Clean up
157+
$this->bookmark->remove(1);
158+
}
159+
160+
public function testCacheInvalidationOnRemove(): void
161+
{
162+
$this->bookmark->removeAll();
163+
$this->bookmark->add(1);
164+
$this->bookmark->getAll(); // Cache füllen
165+
$reflection = new \ReflectionClass(Bookmark::class);
166+
$prop = $reflection->getProperty('bookmarkCache');
167+
$prop->setAccessible(true);
168+
$this->assertIsArray($prop->getValue($this->bookmark));
169+
170+
$this->bookmark->remove(1); // sollte Cache invalidieren
171+
$this->assertNull($prop->getValue($this->bookmark));
172+
}
173+
174+
public function testCacheInvalidationOnRemoveAll(): void
175+
{
176+
$this->bookmark->removeAll();
177+
$this->bookmark->add(1);
178+
$this->bookmark->add(1);
179+
$this->bookmark->getAll(); // Cache füllen
180+
$reflection = new \ReflectionClass(Bookmark::class);
181+
$prop = $reflection->getProperty('bookmarkCache');
182+
$prop->setAccessible(true);
183+
$this->assertIsArray($prop->getValue($this->bookmark));
184+
185+
$this->bookmark->removeAll();
186+
$this->assertNull($prop->getValue($this->bookmark));
187+
}
91188
}

0 commit comments

Comments
 (0)