Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 591 bytes | effulgentsia |
#22 | 2411143-22.patch | 26.39 KB | effulgentsia |
#15 | interdiff.txt | 1.46 KB | amateescu |
#15 | 2411143-15.patch | 26.29 KB | amateescu |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedStarter patch. This will certainly fail.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedComment #3
effulgentsia CreditAttribution: effulgentsia commentedFrom #2407505-19: [meta] Finalize the menu links (and other user-entered paths) system:
#1 does not yet add
description
.Regarding "uri (or path)", the patch does:
What would be nice to settle in this issue is whether we want to store bare paths in here, or always convert to a full URI. Some things to consider:
Drupal\Core\TypedData\Plugin\DataType\Uri
documents the 'uri' data type as being an absolute URI only, but AFAICT doesn't enforce that. We can choose to either change the docs or add enforcement.blog
, we can store that asdrupal://blog
(orinternal://blog
or whatever other scheme name we come up with). The widget can strip that scheme name, so the user never needs to see it.base://
scheme when wanting to link to an internal, but non-Drupal-routed, resource. I think it should mean "internal to the site, but may or may not be handled by Drupal" (see #2405551-51: Add a method to support UIs where users enter paths instead of route names and other valid use cases). This issue does not necessarily need to settle that, but the decision may affect how we want to name the scheme if we decide to make the URI property always stored as absolute.Comment #5
larowlanpoking at fails
Comment #6
larowlankicking it along, fixes some fails - didn't look at migrate fails yet, still some in LinkFieldTest too
Comment #8
dawehnerWorking a bit on the failures.
Comment #9
dawehnerMh, the RDF tests are odd, I don't even get how they pass in HEAD>
Comment #11
dawehnerYou know there are test failures you spent so much time on, you actually hope its a complex problem.
Comment #14
hussainwebI think it didn't set back to Needs review after the tests passed.
Comment #15
amateescu CreditAttribution: amateescu commentedFixed a few small nitpicks and now the patch looks good to go.
Comment #16
Wim LeersFrom #3:
The current patch also doesn't do this yet.
This hasn't been discussed yet.
Both of those things can be dealt with in another issue though; this is at least progress :)
Comment #17
amateescu CreditAttribution: amateescu commentedOops, I didn't mean to jump the gun. It looks like #3.2 should at least be settled in here.
Comment #18
Wim LeersThen this is effectively blocked on the discussion effulgentsia mentioned, in #2405551: Add a method to support UIs where users enter paths instead of route names and other valid use cases.
Comment #19
dawehnerSo the patch is at least consistent with the core behaviour which means: 'string'. If we would want to use something else than 'string'
we would have to adapt typed data to allow so. Not sure whether this really a blocker, or rather an additional issue.
Note: #3.2 was written against 'uri' being there.
Comment #21
hussainwebIt seems like the failure is actually random. It couldn't copy a temporary file for migration and hance the error. I am not retesting it and leaving it for needs work as per #17 and #19.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedI'm ok with punting #3.2 (and #3.1) to a separate issue. Opened #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme for #3.2. Here's a patch that links to it.
Comment #23
amateescu CreditAttribution: amateescu commentedThanks @effulgentsia, I think we can move forward now.
Comment #24
larowlanAre we sure modules/link is the right place for this - not Drupal\Core\Field?
Shortcut module and menu_ui will need to depend on link with it as-is?
Or is that the next step?
Comment #25
dawehnerValid point, but itself AFAIK a major discussion.
I would say its core, it will be maybe used by contributed as well
Comment #26
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 17f5f48 and pushed to 8.0.x. Thanks!
Comment #28
pwolanin CreditAttribution: pwolanin commentedWe need a description column also - is there an issue for that?
Comment #29
Wim LeersAnd now also filed #2413029: Change LinkItem schema to also store a description for #3.1.
Comment #31
podarokthis one was tough for upgrade path
For been able to keep data within link's tables - you should rename all field*_url tables to field*_uri and recreate cache.