Title says it all. We should remove this constant and replace with an '8.x' string. This is not hard to maintain and messes with unit tests.

CommentFileSizeAuthor
#20 2067017-20.patch55.66 KBdamiankloip
#20 interdiff-2067017-20.txt41.43 KBdamiankloip
#16 2067017-16.patch15.36 KBdamiankloip
#16 interdiff-2067017-16.txt6.96 KBdamiankloip
#15 2067017-15.patch8.39 KBdamiankloip
#15 interdiff-2067017-15.txt7.42 KBdamiankloip
#8 2067017-8.patch1000 bytesdamiankloip
#8 interdiff-2067017-8.txt349 bytesdamiankloip
#6 2067017-6.patch1007 bytesdamiankloip
#4 vdc.remove-view-constant.patch562 bytesdamiankloip
#1 vdc.constants-bootstrap.patch2.15 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
StatusFileSize
new vdc.constants-bootstrap.patch2.15 KB

Status: Needs review » Needs work

The last submitted patch, vdc.constants-bootstrap.patch, failed testing.

damiankloip’s picture

damiankloip commented
Component: views.module » base system
damiankloip’s picture

damiankloip commented
Title: Convert DRUPAL_CORE_COMPATIBILITY and VERSION constants in bootstrap.inc to Drupal class constants » Remove usage of DRUPAL_CORE_COMPATIBILITY constant in Drupal\views\Plugin\Core\Entity\View
Component: base system » views.module
Status: Needs work » Needs review
StatusFileSize
new vdc.remove-view-constant.patch562 bytes

Forget it, Let's move it back to just views.

damiankloip’s picture

damiankloip commented
Issue summary: View changes

Updated issue summary.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/includes/bootstrap.inc
@@ -31,12 +31,12 @@
-const VERSION = '8.0-dev';
+const VERSION = Drupal::VERSION;
...
-const DRUPAL_CORE_COMPATIBILITY = '8.x';
+const DRUPAL_CORE_COMPATIBILITY = Drupal::CORE_COMPATIBILITY;

What about just removing that change ... then there is no installer problem.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2067017-6.patch1007 bytes

ok, and just do this?

Status: Needs review » Needs work

The last submitted patch, 2067017-6.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2067017-8.txt349 bytes
new 2067017-8.patch1000 bytes

Oops.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 2067017-8.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
Issue tags: +VDC

#8: 2067017-8.patch queued for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Related #2053519: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes"

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

How many instances of the other constants do exist? Maybe we should ask someone whether they should be converted as well. Is there a tag for it?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Why only remove it from this one file? If we're moving it to the Drupal class, we ought to update all instances everywhere, no? Else we risk updating it in one place and not the other.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review
damiankloip’s picture

damiankloip commented
Title: Remove usage of DRUPAL_CORE_COMPATIBILITY constant in Drupal\views\Plugin\Core\Entity\View » Remove usage of DRUPAL_CORE_COMPATIBILITY constant in classes
StatusFileSize
new interdiff-2067017-15.txt7.42 KB
new 2067017-15.patch8.39 KB

ok, fair enough. How about this?

damiankloip’s picture

damiankloip commented
Title: Remove usage of DRUPAL_CORE_COMPATIBILITY constant in classes » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constant in classes
StatusFileSize
new interdiff-2067017-16.txt6.96 KB
new 2067017-16.patch15.36 KB

Oh, and VERSION too as I've added that to Drupal too?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I think this is good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

Are we sure that we want to maintain the constants in the Drupal class and bootstrap.inc - this seems are bit weird.

Is it not possible just to switch everything to use the class constants?

damiankloip’s picture

damiankloip commented

Well, I think some constants are used before the class loader is initialized, so I don't think so...

If this is going to be an issue, I'm totally fine with just using the patch in #4, so at least I am happier for views tests. But it seems like other classes might as well get fixed along the way too. Plus, come on, that is not alot of code to maintain ;)

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-2067017-20.txt41.43 KB
new 2067017-20.patch55.66 KB

OK, spoke to catch on IRC - We agreed that really there is no need for the current constants to exist, and their usage is pretty much limited to core anyway. So we can remove VERSION and DRUPAL_CORE_COMPATIBILITY and use the Drupal::* alterantives..

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.php
@@ -12,9 +12,6 @@
 // @todo Remove this once the constant got converted.
-if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
-  define('DRUPAL_CORE_COMPATIBILITY', '8.x');
-}

is not @todo for this constant?

damiankloip’s picture

damiankloip commented

There are 2 other constant declarations below this one.

damiankloip’s picture

damiankloip commented
Title: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constant in classes » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

So, this is pretty solid.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants » Change notice: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record
damiankloip’s picture

damiankloip commented
Status: Active » Needs review

OK, done: https://drupal.org/node/2082661

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Retitled change notice from "have been removed" to "have moved". They are not removed, you can find them at a different place.

damiankloip’s picture

damiankloip commented

Fair enough.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Change notice: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good, although, ugh. #2053489: Standardize on \Drupal throughout core

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So this has broken Drush - see https://github.com/drush-ops/drush/pull/65

greg.1.anderson’s picture

greg.1.anderson commented

The problem isn't so much that Drush has broken; Drush can change. The problem is that the fix is awkward for Drush.

See: https://github.com/drush-ops/drush/pull/65

Could we perhaps put the version in composer.json, as David Reid suggested? This is a standard attribute that's widely used; seems like it would be very valuable for Drupal to maintain its version here as well, for constancy. Side benefit: it would be good for Drush.

greg.1.anderson’s picture

greg.1.anderson commented

I created a follow-on issue, #2082931: Store Drupal version in composer.json .

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.