chore

Author: tchapi

.github/workflows/ci.yml

@@ -200,6 +200,9 @@ jobs:
           php bin/console doctrine:database:create --if-not-exists --env=test
           php bin/console doctrine:migrations:migrate --no-interaction --env=test
           php bin/console doctrine:schema:validate --env=test
+          # An empty schema only proves the DDL is valid. Seed the kind of row a DAV client
+          # creates (optional columns left NULL) so the rollback is exercised against data too.
+          php bin/console dbal:run-sql "INSERT INTO addressbooks (principaluri, uri, synctoken) VALUES ('principals/ci', 'ci-rollback-probe', 1)" --env=test
           # Full chain check: roll every migration back down, then all the way up again.
           # Nothing else ever runs the down() methods, so a broken rollback (wrong column
           # name, duplicated ALTER clause, invalid cast) stays invisible until an operator
@@ -216,6 +219,9 @@ jobs:
           php bin/console doctrine:database:create --if-not-exists --env=test
           php bin/console doctrine:migrations:migrate --no-interaction --env=test
           php bin/console doctrine:schema:validate --skip-sync --env=test
+          # An empty schema only proves the DDL is valid. Seed the kind of row a DAV client
+          # creates (optional columns left NULL) so the rollback is exercised against data too.
+          php bin/console dbal:run-sql "INSERT INTO addressbooks (id, principaluri, uri, synctoken) VALUES (nextval('addressbooks_id_seq'), 'principals/ci', 'ci-rollback-probe', 1)" --env=test
           # Full chain check: roll every migration back down, then all the way up again.
           # Nothing else ever runs the down() methods, so a broken rollback (wrong column
           # name, duplicated ALTER clause, invalid cast) stays invisible until an operator
@@ -231,6 +237,9 @@ jobs:
         run: |
           php bin/console doctrine:migrations:migrate --no-interaction --env=test
           php bin/console doctrine:schema:validate --skip-sync --env=test
+          # An empty schema only proves the DDL is valid. Seed the kind of row a DAV client
+          # creates (optional columns left NULL) so the rollback is exercised against data too.
+          php bin/console dbal:run-sql "INSERT INTO addressbooks (principaluri, uri, synctoken) VALUES ('principals/ci', 'ci-rollback-probe', 1)" --env=test
           # Full chain check: roll every migration back down, then all the way up again.
           # Nothing else ever runs the down() methods, so a broken rollback (wrong column
           # name, duplicated ALTER clause, invalid cast) stays invisible until an operator

migrations/Version20191203111729.php

@@ -28,6 +28,9 @@ public function down(Schema $schema): void
     {
         $this->skipIf('mysql' !== $this->connection->getDatabasePlatform()->getName(), 'This migration is specific to \'mysql\'. Skipping it is fine.');
 
-        $this->addSql('ALTER TABLE addressbooks CHANGE description description LONGTEXT CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci`');
+        // Since up() made the column nullable, address books created in the meantime may have
+        // no description at all; they would violate the restored NOT NULL.
+        $this->addSql("UPDATE addressbooks SET description = '' WHERE description IS NULL");
+        $this->addSql('ALTER TABLE addressbooks CHANGE description description LONGTEXT NOT NULL');
     }
 }

migrations/Version20260908220000.php

@@ -0,0 +1,61 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Allow addressbooks.displayname to be null.
+ */
+final class Version20260908220000 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Allow addressbooks.displayname to be null, as a display name is optional when a client creates an address book';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $engine = $this->connection->getDatabasePlatform()->getName();
+
+        // A display name is optional in CardDAV: \Sabre\CardDAV\Backend\PDO::createAddressBook()
+        // binds NULL when the client's MKCOL carries no {DAV:}displayname, and updateAddressBook()
+        // does the same when a PROPPATCH removes it. With a NOT NULL column both fail (HTTP 500).
+        if ('mysql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks CHANGE displayname displayname VARCHAR(255) DEFAULT NULL');
+        } elseif ('postgresql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks ALTER COLUMN displayname DROP NOT NULL');
+        } elseif ('sqlite' === $engine) {
+            // SQLite cannot alter a column in place: add the replacement, copy, swap, drop.
+            $this->addSql('ALTER TABLE addressbooks ADD COLUMN new_displayname VARCHAR(255) DEFAULT NULL');
+            $this->addSql('UPDATE addressbooks SET new_displayname = displayname');
+            $this->addSql('ALTER TABLE addressbooks RENAME COLUMN displayname TO old_displayname');
+            $this->addSql('ALTER TABLE addressbooks RENAME COLUMN new_displayname TO displayname');
+            $this->addSql('ALTER TABLE addressbooks DROP COLUMN old_displayname');
+        }
+    }
+
+    public function down(Schema $schema): void
+    {
+        $engine = $this->connection->getDatabasePlatform()->getName();
+
+        // Address books created without a display name would violate the restored NOT NULL,
+        // so fall back to their uri rather than losing the row.
+        $this->addSql('UPDATE addressbooks SET displayname = uri WHERE displayname IS NULL');
+
+        if ('mysql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks CHANGE displayname displayname VARCHAR(255) NOT NULL');
+        } elseif ('postgresql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks ALTER COLUMN displayname SET NOT NULL');
+        } elseif ('sqlite' === $engine) {
+            // SQLite refuses to ADD a NOT NULL column without a default, so rebuild the table.
+            $this->addSql('CREATE TABLE addressbooks_old (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, principaluri VARCHAR(255) NOT NULL, displayname VARCHAR(255) NOT NULL, uri VARCHAR(255) NOT NULL, description CLOB DEFAULT NULL, synctoken VARCHAR(255) NOT NULL, included_in_birthday_calendar INTEGER DEFAULT 0)');
+            $this->addSql('INSERT INTO addressbooks_old (id, principaluri, displayname, uri, description, synctoken, included_in_birthday_calendar) SELECT id, principaluri, displayname, uri, description, synctoken, included_in_birthday_calendar FROM addressbooks');
+            $this->addSql('DROP TABLE addressbooks');
+            $this->addSql('ALTER TABLE addressbooks_old RENAME TO addressbooks');
+        }
+    }
+}

src/Entity/AddressBook.php

@@ -21,7 +21,7 @@ class AddressBook
     #[ORM\Column(name: 'principaluri', type: 'string', length: 255)]
     private $principalUri;
 
-    #[ORM\Column(name: 'displayname', type: 'string', length: 255)]
+    #[ORM\Column(name: 'displayname', type: 'string', length: 255, nullable: true)]
     private $displayName;
 
     #[ORM\Column(type: 'string', length: 255)]
@@ -73,7 +73,7 @@ public function getDisplayName(): ?string
         return $this->displayName;
     }
 
-    public function setDisplayName(string $displayName): self
+    public function setDisplayName(?string $displayName): self
     {
         $this->displayName = $displayName;
 

src/Form/AddressBookType.php

@@ -24,6 +24,8 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
             ->add('displayName', TextType::class, [
                 'label' => 'form.displayName',
                 'help' => 'form.name.help.carddav',
+                // Optional in CardDAV: clients may create an address book without one
+                'required' => false,
             ])
             ->add('includedInBirthdayCalendar', ChoiceType::class, [
                 'label' => 'form.includedInBirthdayCalendar',

templates/addressbooks/edit.html.twig

@@ -6,7 +6,7 @@
 {% include '_partials/back_button.html.twig' with { url: path('addressbook_index', {userId: userId}), text: "addressbooks.back"|trans({'user': principal.displayName }) } %}
 
 {% if addressbook.id %}
-<h1 class="display-4 fw-lighter mb-5">{{ "addressbooks.edit"|trans({'name': addressbook.displayName }) }}</h1>
+<h1 class="display-4 fw-lighter mb-5">{{ "addressbooks.edit"|trans({'name': addressbook.displayName ?? addressbook.uri }) }}</h1>
 {% else %}
 <h1 class="display-4 fw-lighter mb-5">{{ "addressbooks.new"|trans }} <small class="text-muted">for {{ principal.displayName }}</small></h1>
 {% endif %}

templates/addressbooks/index.html.twig

@@ -11,7 +11,7 @@
 {% for addressbook in addressbooks %}
     <div class="list-group-item p-3">
         <div class="d-flex w-100 justify-content-between">
-            <h5 class="mb-1 me-auto">{{ addressbook.displayName }}</h5>
+            <h5 class="mb-1 me-auto">{{ addressbook.displayName ?? addressbook.uri }}</h5>
             <div class="me-0 text-right d-md-block d-none">
                 <a href="{{ path('addressbook_edit',{userId: userId, id: addressbook.id})}}" class="btn btn-sm btn-outline-primary ms-1">✎ {{ "edit"|trans }}</a>
                 <a href="#"

tests/Functional/AddressBookDavTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Functional;
+
+use Doctrine\ORM\EntityManagerInterface;
+use Sabre\CardDAV\Backend\PDO as CardDavBackend;
+use Sabre\DAV\PropPatch;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+
+class AddressBookDavTest extends KernelTestCase
+{
+    private const PRINCIPAL = 'principals/test_user';
+
+    private EntityManagerInterface $em;
+    private CardDavBackend $backend;
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+
+        $this->em = static::getContainer()->get(EntityManagerInterface::class);
+        $this->backend = new CardDavBackend($this->em->getConnection()->getNativeConnection());
+
+        $this->em->getConnection()->beginTransaction();
+    }
+
+    protected function tearDown(): void
+    {
+        $this->em->getConnection()->rollBack();
+        parent::tearDown();
+    }
+
+    private function addressBookFor(string $uri): array
+    {
+        foreach ($this->backend->getAddressBooksForUser(self::PRINCIPAL) as $book) {
+            if ($uri === $book['uri']) {
+                return $book;
+            }
+        }
+
+        $this->fail(sprintf('No address book found for uri "%s"', $uri));
+    }
+
+    /**
+     * Regression test for issue #275: a display name is optional in CardDAV, but the column
+     * was NOT NULL, so an MKCOL without {DAV:}displayname failed with a 500.
+     */
+    public function testAddressBookCanBeCreatedWithoutADisplayName(): void
+    {
+        $this->backend->createAddressBook(self::PRINCIPAL, 'nameless', []);
+
+        $this->assertNull($this->addressBookFor('nameless')['{DAV:}displayname']);
+    }
+
+    public function testADisplayNameIsStillStoredWhenGiven(): void
+    {
+        $this->backend->createAddressBook(self::PRINCIPAL, 'named', ['{DAV:}displayname' => 'My contacts']);
+
+        $this->assertSame('My contacts', $this->addressBookFor('named')['{DAV:}displayname']);
+    }
+
+    /**
+     * Same column, the other way round: a client may remove the display name with a PROPPATCH.
+     */
+    public function testADisplayNameCanBeRemovedAgain(): void
+    {
+        $id = $this->backend->createAddressBook(self::PRINCIPAL, 'transient', ['{DAV:}displayname' => 'Temporary']);
+
+        $propPatch = new PropPatch(['{DAV:}displayname' => null]);
+        $this->backend->updateAddressBook($id, $propPatch);
+
+        $this->assertTrue($propPatch->commit(), 'The PROPPATCH should succeed');
+        $this->assertNull($this->addressBookFor('transient')['{DAV:}displayname']);
+    }
+}

tests/Functional/Controllers/AddressBookControllerTest.php

@@ -183,4 +183,27 @@ public function testAddressBookNewIgnoresASubmittedOwner(): void
         $addressbookRepository = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(AddressBook::class);
         $this->assertNull($addressbookRepository->findOneBy(['uri' => 'hijack']));
     }
+
+    public function testAddressBookWithoutADisplayNameFallsBackToItsUri(): void
+    {
+        $user = new AdminUser('admin', 'test');
+
+        $client = static::createClient();
+        $client->loginUser($user);
+
+        $userId = $this->getUserId($client, 'test_user');
+
+        $em = static::getContainer()->get('doctrine.orm.entity_manager');
+        $nameless = (new AddressBook())
+            ->setPrincipalUri('principals/test_user')
+            ->setUri('nameless-book')
+            ->setDisplayName(null);
+        $em->persist($nameless);
+        $em->flush();
+
+        $client->request('GET', '/addressbooks/'.$userId);
+
+        $this->assertResponseIsSuccessful();
+        $this->assertAnySelectorTextContains('h5', 'nameless-book');
+    }
 }