Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php (those two seem okay to combine in one patch).

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 interdiff.txt1.05 KBdawehner
#13 2568603-13.patch2.36 KBdawehner
#5 2568603-3.patch2.46 KBdawehner
#5 Screen Shot 2015-09-15 at 13.47.41.png243.18 KBdawehner
#5 Screen Shot 2015-09-15 at 13.47.41.png243.18 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Related issues: +#2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Assigned: Unassigned » dawehner

Let's get started

pguillard’s picture

pguillard CreditAttribution: pguillard commented
Status: Active » Needs review
FileSize
replace_remaining_placeholder_in_tasks-2568603-4.patch1.68 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
FileSize
Screen Shot 2015-09-15 at 13.47.41.png243.18 KB
Screen Shot 2015-09-15 at 13.47.41.png243.18 KB
2568603-3.patch2.46 KB

Here is a screenshot with all of them hacked in temporarily.

Just in case someone asked, we could use (new Link()) but therefore we need to fix the UnroutedUrlAssembler, see #2568773: Add $options['base_url'] support to the unrouted URL assembler

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Assigned: dawehner » Unassigned

@pguillard
Did you tried whether this actually works, because I doubt it.

pguillard’s picture

pguillard CreditAttribution: pguillard commented

@dawehner : no, I didn't try

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

@pguillard
So yeah in your example the <code> would have been escaped and by that not what you actually want. You want $query to be HTML wrapped.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs manual testing
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -122,10 +122,10 @@ protected function checkEncoding() {
+        $base_url = str_replace('core/install.php', 'core/', \Drupal::request()->getBaseUrl());
+        $this->fail(t('The %driver database must use %encoding encoding to work with Drupal. Recreate the database with %encoding encoding. See <a href="/core/INSTALL.pgsql.txt">INSTALL.pgsql.txt</a> for more details.', array(

That looks like it wants a test added too.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes

(Embedding @dawehner's screenshots.)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Needs work
Issue tags: -Needs manual testing +Needs tests 
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -122,10 +122,10 @@ protected function checkEncoding() {
-        $this->fail(t('The %driver database must use %encoding encoding to work with Drupal. Recreate the database with %encoding encoding. See !link for more details.', array(
+        $base_url = str_replace('core/install.php', 'core/', \Drupal::request()->getBaseUrl());
+        $this->fail(t('The %driver database must use %encoding encoding to work with Drupal. Recreate the database with %encoding encoding. See <a href="/core/INSTALL.pgsql.txt">INSTALL.pgsql.txt</a> for more details.', array(
           '%encoding' => 'UTF8',
           '%driver' => $this->name(),
-          '!link' => '<a href="INSTALL.pgsql.txt">INSTALL.pgsql.txt</a>'

So that $base_url is not being used... is the link broken, or not?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Novice, -Needs tests
FileSize
2568603-13.patch2.36 KB
interdiff.txt1.05 KB

Thank you for your reviews xjm!!

I fear testing this will be incredible hard. You would basically have to compile a Mysql server without utfmb4 support. PHP is a programming language, so yes, we could do that,
but feels a little bit too much as requirement for this issue. The same was decided on the utfmb4 issue which introduced this particular code.

So that $base_url is not being used... is the link broken, or not?

Ah yeah I was experimenting with the url generator code. Let's keep it as straightforward as possible and use exactly the same as HEAD, which works!
You link to a relative URL, and "core" is the current base path as seen by your browser. Everything is fine. I tested it on d8.dev and localhost/d8 and clicked on the link.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Reviewed & tested by the community

I fear testing this will be incredible hard. You would basically have to compile a Mysql server without utfmb4 support. PHP is a programming language, so yes, we could do that, but feels a little bit too much as requirement for this issue. The same was decided on the utfmb4 issue which introduced this particular code.

LOL fair enough.

This seems completely sufficient to me then. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 333977e and pushed to 8.0.x. Thanks!

  • alexpott committed 333977e on 
    Issue #2568603 by dawehner, xjm, pguillard: Replace remaining !...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Parent issue: » #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only
Related issues: -#2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

Status: Fixed » Closed (fixed)

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