Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

FileSize
3.09 KB

Starter patch. This will certainly fail.

effulgentsia’s picture

Status: Active » Needs review
effulgentsia’s picture

From #2407505-19: [meta] Finalize the menu links (and other user-entered paths) system:

The link field would have 4 columns: title, description, uri (or path), and options.

#1 does not yet add description.

Regarding "uri (or path)", the patch does:

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -44,18 +44,12 @@ public static function defaultFieldSettings() {
-    $properties['url'] = DataDefinition::create('string')
-      ->setLabel(t('URL'));
+    $properties['uri'] = DataDefinition::create('uri')
+      ->setLabel(t('URI'));

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:

  • http://en.wikipedia.org/wiki/Uniform_resource_identifier#URI_reference uses the term "URI reference" to describe a string that can be either an absolute URI or some relative portion of one.
  • 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.
  • If we want to use the 'uri' type and enforce it as being absolute, we can invent yet another scheme for when the user types a bare path. For example, if the user types blog, we can store that as drupal://blog (or internal://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.
  • @pwolanin and I are still disagreeing in #2405551: Add a method to support UIs where users enter paths instead of route names and other valid use cases as to what the meaning of a schemeless path (or the above scheme) should be. He thinks it should always mean "routed by Drupal", requiring the user to explicitly use the 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.

Status: Needs review » Needs work

The last submitted patch, 1: 2411143.1.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan

poking at fails

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
19.31 KB
22.07 KB

kicking it along, fixes some fails - didn't look at migrate fails yet, still some in LinkFieldTest too

Status: Needs review » Needs work

The last submitted patch, 6: menu-links-stuff-2411143.2.patch, failed testing.

dawehner’s picture

Assigned: Unassigned » dawehner

Working a bit on the failures.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
26.21 KB
6.48 KB

Mh, the RDF tests are odd, I don't even get how they pass in HEAD>

Status: Needs review » Needs work

The last submitted patch, 9: 2411143-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.63 KB
753 bytes

You know there are test failures you spent so much time on, you actually hope its a complex problem.

Status: Needs review » Needs work

The last submitted patch, 11: 2411143-11.patch, failed testing.

dawehner queued 11: 2411143-11.patch for re-testing.

hussainweb’s picture

Status: Needs work » Needs review

I think it didn't set back to Needs review after the tests passed.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
26.29 KB
1.46 KB

Fixed a few small nitpicks and now the patch looks good to go.

Wim Leers’s picture

From #3:

  1. #1 does not yet add description.

    The current patch also doesn't do this yet.

  2. 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:

    This hasn't been discussed yet.

Both of those things can be dealt with in another issue though; this is at least progress :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs review

Oops, I didn't mean to jump the gun. It looks like #3.2 should at least be settled in here.

Wim Leers’s picture

Then 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.

dawehner’s picture

So 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.

Status: Needs review » Needs work

The last submitted patch, 15: 2411143-15.patch, failed testing.

hussainweb’s picture

It 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.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
26.39 KB
591 bytes

I'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.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @effulgentsia, I think we can move forward now.

larowlan’s picture

Are 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?

dawehner’s picture

Valid point, but itself AFAIK a major discussion.

I would say its core, it will be maybe used by contributed as well

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 17f5f48 on 8.0.x
    Issue #2411143 by dawehner, effulgentsia, amateescu, larowlan: Change...
pwolanin’s picture

We need a description column also - is there an issue for that?

Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

podarok’s picture

this 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.